-
Notifications
You must be signed in to change notification settings - Fork 55
fix(#507): warn when CLI defaults conflict with TOML #512
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #512 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 15 15
Lines 966 970 +4
Branches 170 171 +1
=========================================
+ Hits 966 970 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/mdformat/_cli.py
Outdated
| plugin_file, plugin_line = get_source_file_and_line(action) | ||
| warnings.warn_explicit( | ||
| text, | ||
| UserWarning, |
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.
Let's use DeprecationWarning to avoid the visibility issue mentioned here #507 (comment)
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.
The risk with a deprecation warning is that developers may never see it because the test code needs to exercise run() or the plugin author needs to modify the warning level, which when tested with my prior release of mdformat-mkdocs wouldn't have been visible to me
I think a UserWarning might be appropriate because ultimately users will be the ones experiencing unexpected behavior. The impact should be small because only plugins that have migrated to the new groups and have arguments with defaults will need to make any changes. For examples mdformat-tables=1.0.0 won't error and I don't see other examples on GitHub: https://github.com/search?q=add_cli_argument_group+language%3APython+NOT+repo%3Ahukkin%2Fmdformat+NOT+repo%3Ajamesquilty%2Fmdformat+NOT+repo%3Aantazoey%2Fmdformat+NOT+repo%3Ajamesquilty%2Fmdformat-tables+NOT+repo%3Ahukkin%2Fmdformat-tables+NOT+repo%3AKyleKing%2Fmdformat&type=code&l=Python
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.
Besides potential unexpected behavior, the user is also the one experiencing the annoying warning message. But unlike the developer, the user cannot do anything about it. There can be plugins that decide not to read from the configuration file, and options where TOML configuration makes no sense. Mdformat would show a warning to users, though working exactly as intended.
If it's later decided that a hard error here is better, a DeprecationWarning is more accurate at signaling future removal.
So I'd say let's keep the DeprecationWarning. That, to my knowledge, is visible by default in pytest and unittest. I encourage plugin developers to test with mdformat._cli.run.
KyleKing
left a comment
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.
Thanks for the feedback!
Co-authored-by: Taneli Hukkinen <[email protected]>
src/mdformat/_cli.py
Outdated
| plugin_file, plugin_line = get_source_file_and_line(action) | ||
| warnings.warn_explicit( | ||
| text, | ||
| UserWarning, |
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.
Besides potential unexpected behavior, the user is also the one experiencing the annoying warning message. But unlike the developer, the user cannot do anything about it. There can be plugins that decide not to read from the configuration file, and options where TOML configuration makes no sense. Mdformat would show a warning to users, though working exactly as intended.
If it's later decided that a hard error here is better, a DeprecationWarning is more accurate at signaling future removal.
So I'd say let's keep the DeprecationWarning. That, to my knowledge, is visible by default in pytest and unittest. I encourage plugin developers to test with mdformat._cli.run.
|
Merged, thank you! 🎉 |
Fixes #507
Adds a user-warning when plugins have argument defaults that conflict with the new TOML config
Edit: the example STDERR above was updated to reflect the implementation changes