[ENH] respect explicit default in get_tag to match getattr semantics#555
[ENH] respect explicit default in get_tag to match getattr semantics#555Deep-Axe wants to merge 2 commits intosktime:mainfrom
Conversation
for more information, see https://pre-commit.ci
fkiraly
left a comment
There was a problem hiding this comment.
Hm, can you kindly explain why you think this change is needed, or why it would be an improvement? In which usage scenario, etc.
I also want to point out that this is not backwards compatible and in a quite central place in the API.
Namely, in case that raise_error=True and passing tag_value_default, the behaviour changes.
|
Hi @fkiraly The PR does change behavior for the case where a caller passes As you suggested in the original issue thread, a deprecation cycle would be a place to start. |
Reference Issues/PRs
Fixes #291
What does this implement/fix? Explain your changes.
This PR modifies
get_tag(and the internal_get_flag) to respect an explicitly providedtag_value_default. Currently,get_tagraises aValueErrorif a tag is missing even if a default value is supplied, unlessraise_error=Falseis also explicitly passed. This behavior is unintuitive, and inconsistent withget_class_tagand standard patterns likegetattr()ordict.get()Changes:
_DEFAULT_SENTINELin_tagmanager.pyto reliably distinguish between an omitted default and an explicit None default._get_flagto bypass theraise_errorcheck if a default was provided._base.pyand_tagmanager.pyto clarify the new behavior.test_base.pyto verify that explicit defaults (including None) correctly suppress the ValueError without needing raise_error=False.Backward Compatibility:
The change is fully backward compatible. Calling
get_tag("missing")without a second argument will still raise a ValueError by default, preserving current error-checking behavior for typos.Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
_DEFAULT_SENTINELobject to ensureget_tag("name", None)correctly returns None instead of raising an error.get_taglogic with the existing behavior ofget_class_tag.Any other comments?
As discussed in the issue thread, a follow-up PR could be opened later to start a
FutureWarningdeprecation cycle for changing the global default ofraise_errorfrom True to False, but this PR focuses on respecting the user-supplied default first.PR checklist
For all contributions
For code contributions