Drop deadline warning in dev.deprecated
#735
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #735 +/- ##
==========================================
+ Coverage 84.19% 84.40% +0.20%
==========================================
Files 27 27
Lines 1670 1654 -16
Branches 314 313 -1
==========================================
- Hits 1406 1396 -10
+ Misses 202 198 -4
+ Partials 62 60 -2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/test_dev.py (2)
172-175: Ensure consistency in the test message.
The phrase "This function should have been removed" in thematchargument is correct for deprecated functions, but consider using language inclusive of classes if the decorator is used on class objects as well. This will help future-proof your tests if you later expand coverage.
185-187: Use a more descriptive filter message.
You're filtering specifically for the phrase "function should have been removed," but this might be confusing if the decorator is applied to classes or other entities in the future. Consider adjusting the match string for broader coverage, such as "should have been removed" without the word "function."src/monty/dev.py (2)
36-37: Minor docstring refinement.
Consider highlighting that the deadline tuple is(year, month, day). It may be worth clarifying whether zero-padded or leading formats matter.
112-113: Deadline conversion validation.
It might be beneficial to handle potentialValueErrorthrown if users pass invalid date tuples (e.g.,(2025, 13, 1)). Currently, aValueErrorfromdatetime(*deadline)would bubble up in an unclear manner.if deadline is not None: try: _deadline: datetime = datetime(*deadline) except ValueError as e: + raise ValueError(f"Invalid deadline: {deadline}") from e else: _deadline = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/monty/dev.py(3 hunks)tests/test_dev.py(1 hunks)
🔇 Additional comments (4)
tests/test_dev.py (3)
177-179: Confirm behavior for post-deadline usage.
Decoratingfunc_oldwith a past date ensures theDeprecationWarningis raised. Looks good for verifying deadline-based deprecation. Ensure that the replacement or additional documentation provides guidance on alternatives.
181-182: Good practice to clarify test purpose.
The docstring is clear that no warning should be raised before the deadline. Tests confirm the new approach of ignoring the repository check.
189-190: No-warn scenario verification.
This test correctly validates that no deprecation warning is raised before the far-future deadline. Smoothly confirms that the logic gracefully skips warnings in borderline scenarios.src/monty/dev.py (1)
46-46: Docstring alignment looks good.
The line succinctly states what the decorator returns and is consistent with the updated behavior (now issuing direct warnings).
DanielYang59
changed the title
dev.deprecated and warn instead of raisedev.deprecated and warn instead of raise
DanielYang59
force-pushed
the
fix-deprecated-repo-check
branch
from
296466b to
19ed737
Compare
DanielYang59
force-pushed
the
fix-deprecated-repo-check
branch
3 times, most recently
from
9b31428 to
86568d6
Compare
DanielYang59
force-pushed
the
fix-deprecated-repo-check
branch
from
86568d6 to
279b265
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/monty/dev.py (2)
38-39: Docstring accurately reflects the warning behavior changeThe docstring updates correctly reflect the switch from raising exceptions to emitting warnings. However, consider making the docstring more inclusive by mentioning both functions and classes explicitly, as this decorator can be applied to both.
- of the old function/class, in format (yyyy, MM, dd). DeprecationWarning + of the deprecated entity, in format (yyyy, MM, dd). DeprecationWarningAlso applies to: 48-48
54-57: Enhance docstring clarity for repository check requirementsThe docstring could be more explicit about the requirements for the repository check to work correctly.
- """Check if is running in code owner's repo. - Only generate reliable check when `git` is installed and remote name - is "upstream". - """ + """Check if the code is running in the original repository (not a fork). + + Requirements: + - Git must be installed + - Repository must have a remote named 'upstream' pointing to the original repository + - GITHUB_REPOSITORY environment variable must be set (typically in CI) + """
🛑 Comments failed to post (1)
src/monty/dev.py (1)
158-159:
⚠️ Potential issueReview warning emission timing
The
emit_deadline_warning()is called during decorator application rather than when the deprecated entity is used. This means the warning will be emitted when the module is imported, which might not be the intended behavior.Consider moving the warning emission into the wrapped function/class:
- # Emit CI warning after removal deadline - emit_deadline_warning() def decorator(target: Callable) -> Callable: if inspect.isfunction(target): + emit_deadline_warning() # Move inside the wrapped function return deprecated_function_decorator(target) elif inspect.isclass(target): + emit_deadline_warning() # Move inside the class initialization return deprecated_class_decorator(target)Committable suggestion skipped: line range outside the PR's diff.
DanielYang59
changed the title
dev.deprecated and warn instead of raisedeadline warning in dev.deprecated
DanielYang59
force-pushed
the
fix-deprecated-repo-check
branch
from
ab30ec7 to
740933b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/monty/dev.py (2)
33-33: Enhance docstring clarity for better developer experience.The docstring could be more explicit about:
- The deadline format - specify that it expects a tuple of integers (yyyy, MM, dd)
- The return value behavior - clarify that the warning is shown on each invocation
- of the old function/class, in format (yyyy, MM, dd). + of the old function/class. Expected format: tuple of integers (yyyy, MM, dd), + e.g., (2024, 12, 31). - Original function/class, but with a warning to use the replacement. + Original function/class wrapped with a warning that will be shown on each + invocation, directing users to the replacement if specified.Also applies to: 42-42
Line range hint
1-174: Approval: Simplified warning mechanism improves downstream usage.The removal of CI-specific warning logic is a good architectural decision that addresses the issue with downstream function calls. This change makes the deprecation mechanism more straightforward and easier to manage for downstream dependencies.
Consider documenting this architectural change in the module's docstring to explain the simplified warning behavior for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/monty/dev.py(3 hunks)src/monty/logging.py(0 hunks)tests/test_dev.py(6 hunks)
💤 Files with no reviewable changes (1)
- src/monty/logging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_dev.py
🔇 Additional comments (1)
src/monty/dev.py (1)
Line range hint
52-71: Use generic terminology in warning messages.As noted in previous reviews, the
@deprecateddecorator is used on both functions and classes. The warning message should use generic terminology like "entity" or "item" instead of specifically mentioning "function/class".- msg = f"{old.__name__} is deprecated" + msg = f"This entity ({old.__name__}) is deprecated"
|
@shyuep Sorry for the first major issue of 2025, looks like it's broadly impacting |
Just to clarify, it would be fine if it emitted a warning via |
|
Thanks @shyuep for the quick release :)
This point is well taken, but it's still broadcasting that warning to undesired audience (downstream package), so I might only add such a warning if I could somehow confine it only to the desire target (which I don't have a good idea now) |
Summary
deadlinewarning indev.deprecated, link to Removeis_rare_earth_metalmaterialsproject/pymatgen#4243.downstreamusesupstream's deprecated function, a warning would still be emitted indownstream's CI, butdownstreamcannot resolve it)I hope I didn't misunderstand anything, comment appreciated :)