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

Remove bail out behavior if one of the entrypoints does not exist #1165

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ceoro9
Copy link

@ceoro9 ceoro9 commented Jul 31, 2021

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?

@ceoro9 ceoro9 force-pushed the feat/exclude-invalid-entrypoints branch 2 times, most recently from 4b837ec to 52e8bb7 Compare July 31, 2021 10:48
@codecov
Copy link

codecov bot commented Jul 31, 2021

Codecov Report

Merging #1165 (dd22b7d) into develop (09e11bc) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1165   +/-   ##
========================================
  Coverage    91.05%   91.05%           
========================================
  Files           42       42           
  Lines         2593     2593           
  Branches       752      752           
========================================
  Hits          2361     2361           
  Misses         232      232           
Flag Coverage Δ
integration 79.52% <0.00%> (ø)
legacy 55.61% <0.00%> (ø)
unit 49.59% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/runner/extract-runnable-items.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09e11bc...dd22b7d. Read the comment docs.

@codenirvana
Copy link
Member

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.

@ceoro9
Copy link
Author

ceoro9 commented Aug 7, 2021

@codenirvana , I'm totally fine to throw an error. Thank you for response 🙏🏼 I ll modify the code right in the current PR.

@ceoro9 ceoro9 force-pushed the feat/exclude-invalid-entrypoints branch from 52e8bb7 to 9450484 Compare August 7, 2021 09:51
@codenirvana
Copy link
Member

@ceoro9 Actually, there's an abortOnError runner option (refer to this test) which you can set to make sure the run terminates with an error.

@ceoro9
Copy link
Author

ceoro9 commented Aug 10, 2021

@codenirvana , cool. I ll use it. Thank you. Any other comments on code?

@ceoro9 ceoro9 force-pushed the feat/exclude-invalid-entrypoints branch from 9450484 to 98fb5b2 Compare August 10, 2021 10:16
For lookupStrategy "multipleIdOrName", we should bail out wit an error
if one of the provided entrypoints does not exist.
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.

2 participants