Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes GH2626 - removes the null-coalescing assignment operator when … #2638

Conversation

clrudolphi
Copy link
Contributor

…processing items in the Enumerable Value Retriever; which forces the EVR to find and use the most appropriate Value Retriever for each item in the list (instead of assuming that each item can be processed by the ValueRetriever found for the first item in the list).

Types of changes

  • [X ] Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Performance improvement
  • Refactoring (so no functional change)
  • Other (docs, build config, etc)

Checklist:

  • [X ] I've added tests for my code. (most of the time mandatory)
  • I have added an entry to the changelog. (mandatory)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

…processing items in the Enumerable Value Retriever; which forces the EVR to find and use the most appropriate Value Retriever for each item in the list (instead of assuming that each item can be processed by the ValueRetriever found for the first item in the list).
Copy link
Contributor

@SabotageAndi SabotageAndi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. I am fine with it, except the one comment.
Please also add an entry to the changelog.

Modified location of variable declaration in EnumerableValueRetriever to make the code cleaner.
Added this change to the changelog.txt
@bollhals
Copy link
Contributor

Hmm this case is a bit unfortunate, but apart from the null value, is there any example where you'd want to retrieve the same type via 2 valueretrievers? It doesn't make sense to me. I have a ValueRetriever to handle a type, so if I have a enumerable of that type, I expect the one retriever to be able to handle it no?

I'm wondering if it wouldn't be better to extend the Null value handling in the StructValueRetriever
, so you could set a value that is treated as null, like in the NullValueRetriever

@SabotageAndi
Copy link
Contributor

@bollhals I found the example in the issue (#2626) convincing me to accept this change.
At the moment we guess the value retriever from the first row, which is not ideal and a surprising behavior for the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants