-
Notifications
You must be signed in to change notification settings - Fork 14
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
chore: add warnings #317
chore: add warnings #317
Conversation
make IDEs signal deprecated code instead of relying on runtime logs only
WalkthroughThis pull request introduces a comprehensive deprecation warning strategy across multiple files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #317 +/- ##
==========================================
- Coverage 49.89% 49.27% -0.62%
==========================================
Files 37 37
Lines 4341 4426 +85
==========================================
+ Hits 2166 2181 +15
- Misses 2175 2245 +70 ☔ View full report in Codecov by Sentry. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ovos_workshop/skills/layers.py (1)
4-10
: Use consistent deprecation messaging and document recommended alternatives.Shifting from
log_deprecation()
towarnings.warn()
is good for IDE integration, but ensure you specify the recommended alternative or future usage path in the message to help downstream consumers adopt new patterns quickly. Consider explicitly mentioning the correct import path or replacement function.ovos_workshop/decorators/fallback_handler.py (1)
3-9
: Align the warning message with recommended usage.The new warning is helpful, but the message merely mentions the old import path. You may want to include guidance on where to import from instead. A more descriptive message can reduce confusion and shorten migration time.
ovos_workshop/decorators/__init__.py (1)
78-82
: Ensure the deprecation message is explicit.While the generic message references
@intent_handler
, consider clarifying the recommended usage pattern, e.g., "Replace@intent_file_handler(intent_file)
with@intent_handler(intent_file)
". This ensures a frictionless developer experience when migrating older code.ovos_workshop/skills/intent_provider.py (1)
17-21
: Appropriate use of warnings within class constructor.Using
stacklevel=2
ensures that calling code is identified correctly in the traceback. Consider pairing this warning with docstring updates to help inform users about the recommended transition process.ovos_workshop/backwards_compat.py (1)
1-7
: Well-placed deprecation warning at the file level.Notifying developers that this file “will be removed soon” sets clear expectations. However, consider specifying the exact version or date of removal to provide an even more precise roadmap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ovos_workshop/backwards_compat.py
(1 hunks)ovos_workshop/decorators/__init__.py
(2 hunks)ovos_workshop/decorators/converse.py
(1 hunks)ovos_workshop/decorators/fallback_handler.py
(1 hunks)ovos_workshop/intents.py
(3 hunks)ovos_workshop/skill_launcher.py
(1 hunks)ovos_workshop/skills/common_query_skill.py
(2 hunks)ovos_workshop/skills/intent_provider.py
(2 hunks)ovos_workshop/skills/layers.py
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ovos_workshop/skill_launcher.py
🔇 Additional comments (10)
ovos_workshop/decorators/converse.py (1)
2-8
: Confirm the justification for the warning.It's good to issue a
DeprecationWarning
on module import; however, ensure that you have updated the documentation accordingly and provided a clear migration path. This helps developers know which module or function to switch to.ovos_workshop/decorators/__init__.py (1)
4-4
: Keep import statements consistent across all deprecation changes.Adding
warnings
here is consistent with other files. Make sure that all modules implementing deprecation warnings import and log them similarly to avoid confusion.ovos_workshop/skills/intent_provider.py (3)
7-7
: Great addition of thewarnings
import statement.This import is necessary to enable the newly added deprecation notifications.
9-13
: Deprecation warning at module level looks good.Issuing a deprecation warning at the module level effectively raises developer awareness of planned changes, aligning with the PR’s objective to warn about deprecated code.
65-69
: Consistent deprecation strategy inIntentEngineSkill
.This warning mirrors the earlier approach in
BaseIntentEngine
. The approach is consistent and helps unify the codebase’s deprecation flow.ovos_workshop/skills/common_query_skill.py (2)
21-21
: Good practice to importwarnings
.The import is placed in a logical position. No issues here.
63-67
: Clear and user-friendly deprecation message.The message provides a concise migration path, recommending the
@common_query
decorator withOVOSSkill
. Ensure the documentation strongly references this path to guide users seamlessly.ovos_workshop/intents.py (3)
6-6
:warnings
import looks properly introduced.No conflicts or issues detected.
503-507
: Effective nudge towards the newremove_intent
method.The message clearly communicates that the old method is deprecated. Ensure that all references to
detach_intent
in your codebase or documentation are updated accordingly.
622-626
: Deprecation warning forget_intent_names
is concise.Excellent job directing users to the new property-based approach. Verify that any references in downstream code or docs are updated to reduce confusion.
make IDEs signal deprecated code instead of relying on runtime logs only
Summary by CodeRabbit
Release Notes
Deprecation Warnings
Code Maintenance
These changes help guide developers towards more current implementation practices while maintaining existing functionality.