Deprecate colcon_mixin.mixin.repository.mixin_repositories_file - #55
Conversation
Importing this module currently requires that colcon has already initialized the config location, which makes testing difficult and produces undesired behavior if the location changes during runtime. Following the behavior of colcon_mixin.mixin.get_mixin_path, we should use a "getter" pattern here. This change also adds a test for an existing deprecation in this package.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #55 +/- ##
===========================================
+ Coverage 0.00% 15.30% +15.30%
===========================================
Files 11 11
Lines 551 562 +11
Branches 0 94 +94
===========================================
+ Hits 0 86 +86
+ Misses 551 474 -77
- Partials 0 2 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
leander-dsouza
left a comment
There was a problem hiding this comment.
Added some minor comments about the category of warning to be raised.
However, importing set_repositories on its own still leads to an error, as set_default_config_path needs to be called before get_config_path().
| warnings.warn( | ||
| "'colcon_mixin.mixin.repository.mixin_repositories_file' has " | ||
| "been deprecated, use 'colcon_mixin.mixin.repository." | ||
| "get_mixin_repositories_file()' instead", stacklevel=2) |
There was a problem hiding this comment.
| "get_mixin_repositories_file()' instead", stacklevel=2) | |
| "get_mixin_repositories_file()' instead", | |
| DeprecationWarning, stacklevel=2) |
PEP 387 suggests adding DeprecationWarning as the warning category for usage.
In anticipation of this change, I have added suggestions to the tests to match this call.
There was a problem hiding this comment.
This package already uses UserWarning for deprecations elsewhere. The main reason is that DeprecationWarning messages are not shown to on the console and are effectively silent unless specifically enabled: https://docs.python.org/3/library/warnings.html#default-warning-filter
There was a problem hiding this comment.
Thank you for the explanation, Scott. The remaining associated comments are related to this addition and can therefore be ignored.
Co-authored-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
Co-authored-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
Importing this module currently requires that colcon has already initialized the config location, which makes testing difficult and produces undesired behavior if the location changes during runtime. Following the behavior of
colcon_mixin.mixin.get_mixin_path, we should use a "getter" pattern here.This change also adds a test for an existing deprecation in this package.