Skip to content

[ENH] respect explicit default in get_tag to match getattr semantics#555

Open
Deep-Axe wants to merge 2 commits intosktime:mainfrom
Deep-Axe:update-get-tag
Open

[ENH] respect explicit default in get_tag to match getattr semantics#555
Deep-Axe wants to merge 2 commits intosktime:mainfrom
Deep-Axe:update-get-tag

Conversation

@Deep-Axe
Copy link
Copy Markdown

@Deep-Axe Deep-Axe commented May 2, 2026

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 provided tag_value_default. Currently, get_tag raises a ValueError if a tag is missing even if a default value is supplied, unless raise_error=False is also explicitly passed. This behavior is unintuitive, and inconsistent with get_class_tag and standard patterns like getattr() or dict.get()

Changes:

  • Introduced a _DEFAULT_SENTINEL in _tagmanager.py to reliably distinguish between an omitted default and an explicit None default.
  • Updated _get_flag to bypass the raise_error check if a default was provided.
  • Updated public method signatures and docstrings in _base.py and _tagmanager.py to clarify the new behavior.
  • Added unit tests in test_base.py to 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?

  • The use of the _DEFAULT_SENTINEL object to ensure get_tag("name", None) correctly returns None instead of raising an error.
  • The alignment of the get_tag logic with the existing behavior of get_class_tag.

Any other comments?

As discussed in the issue thread, a follow-up PR could be opened later to start a FutureWarning deprecation cycle for changing the global default of raise_error from True to False, but this PR focuses on respecting the user-supplied default first.

PR checklist

For all contributions
  • I've reviewed the project documentation on contributing
  • I've added myself to the list of contributors.
  • The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions
  • Unit tests have been added covering code functionality
  • Appropriate docstrings have been added (see documentation standards)
  • New public functionality has been added to the API Reference

Copy link
Copy Markdown
Contributor

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

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.

@fkiraly fkiraly added implementing framework Implementing core skbase framework enhancement Adding new functionality labels May 3, 2026
@Deep-Axe
Copy link
Copy Markdown
Author

Deep-Axe commented May 3, 2026

Hi @fkiraly
The motivation was from the thread in Issue #291.
get_tag("name","fallback") raises a ValueError even though the caller explicitly provided a fallback.
This defeats the purpose of tag_value_default unless the user also knows to pass raise_error=False. This deviates from standard Python patterns (getattr, dict.get) and is
inconsistent with get_class_tag, which already returns the supplied default unconditionally.

The PR does change behavior for the case where a caller passes tag_value_default but leaves raise_error as its default True.
While this call pattern is contradictory (saying "use this fallback if missing" and "raise an error if missing" simultaneously), but it shouldn't silently change.

As you suggested in the original issue thread, a deprecation cycle would be a place to start.
Would you like me to update this PR so that passing a default while raise_error=True emits a FutureWarning (warning of the future getattr-like behavior) but still raises the ValueError for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Adding new functionality implementing framework Implementing core skbase framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Behaviour of get_tag is different from standard expectation from get or getattr

2 participants