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.
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
FIX
EmptyRequest.get
defaults toBunch
ofMETHODS
#28371FIX
EmptyRequest.get
defaults toBunch
ofMETHODS
#28371Changes from 7 commits
5e94bd1
e56b4d8
6da316e
2aa6c16
940e53d
8ed5630
4bcaab6
6610d7b
d32ca3a
d500ae5
974c559
adf4eff
2ca2aa7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please advise on this part. I raised it earlier in the PR but there's been no comment there.
It's hard to find exact usage without types but by searching for
.get(
across the repo and I found a few places there isparams.get("x", {})
. It's hard to tell ifparams
is adict
or anEmptyRequest
.It might not be a problem yet but I feel like this could silently cause hard to debug issues in the future, especically in cases where you expected a
{}
but instead got aBunch(**{method: {} for method in METHODS})
.First recommendation is use a sentinel value to indicate nothing was passed in. Similar to how
more_itertools
works with defaults. This allows things to work like so:Second recommendation if you do not wish to introduce a sentinel pattern is just to use an explicit
if default is None
check instead of implicit falsyness. However this might not work as expected:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the most intuitive approach here is the sentinel value. Basically, not passing anything will always return a
Bunch
. Settingdefault
will return the type ofdefault
.Such semantic is not surprising and expected. Right now, having
None
returning a Bunch is indeed surprising.I don't know what @adrinjalali thinks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented the sentinel value approach in the meantime, happy to revert it if @adrinjalali thinks this should not be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior should be like a dictionary, when passed the default and the key doesn't exist, we return the default. In this case, I wonder if we should ignore default completely (there only to immitate
dict
), and always return the empty routing list. Afterall, the whole point of this class is to return an empty routed_params object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed at removing the
default
param. However, we would need to change the pattern:that is used in the pipeline for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't remove it, it just always return empty and ignores
default
. Thedefault
needs to be there to mimic adict().get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codecov is not happy here. I need to figure out when is it the case that
default=None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrinjalali I assume that we should be able to cover this one because it would be equivalent to call e.g.
I don't where is the best place to test this. This looks like a metadata routing test to me.