Remove bail out behavior if one of the entrypoints does not exist #1165
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ceoro9
force-pushed
the
feat/exclude-invalid-entrypoints
branch
2 times, most recently
from
4b837ec
to
52e8bb7
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1165 +/- ##
========================================
Coverage 91.05% 91.05%
========================================
Files 42 42
Lines 2593 2593
Branches 752 752
========================================
Hits 2361 2361
Misses 232 232
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Agree that this is an unexpected behavior but I would rather like to bail out early with an error than executing an invalid (with missing items) run. |
|
@codenirvana , I'm totally fine to throw an error. Thank you for response 🙏🏼 I ll modify the code right in the current PR. |
ceoro9
force-pushed
the
feat/exclude-invalid-entrypoints
branch
from
52e8bb7
to
9450484
Compare
ceoro9
commented
Aug 7, 2021
|
@codenirvana , cool. I ll use it. Thank you. Any other comments on code? |
ceoro9
force-pushed
the
feat/exclude-invalid-entrypoints
branch
from
9450484
to
98fb5b2
Compare
For lookupStrategy "multipleIdOrName", we should bail out wit an error if one of the provided entrypoints does not exist.
ceoro9
force-pushed
the
feat/exclude-invalid-entrypoints
branch
from
98fb5b2
to
dd22b7d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If one of the provided entry points is invalid(probably it does exist), then we SHOULDN'T bail out.
Why? Because this is completely surprising behavior for API.
If I have a list of valid entry points(at least I think so, in reality just one of them can be invalid. Mb typo or anything else, whatever). Then I try to run them, I'll get nothing in result. Exactly nothing. Not the error, just success saying nothing was run.
What am I supposed to think of? I see my correct entry points and they're not run. This is so confusing.
The predictable behavior in that case would be throw an error, saying one of your entry points does not exist. Or just ignore the invalid entry points. The correct choice depends on the executed operation. If it's Get(means result should be returned), then error is more natural. If it's Find(like searching for something), then ignoring of non-existent entry points is totally fine.
Since in our case we got a look up(lookupByMultipleIdOrName), then ignoring is our choice.
Btw making some warning about the invalid entry points would be great, do we have a logger or anything like this?