-
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
49352eb
fix(#507): warn on store_true and store_false
KyleKing 9684f2e
fix: more readable plugin name in warning
KyleKing a51512b
refactor: implement code review feedback
KyleKing f003676
Merge branch 'master' into fix-507
KyleKing 55db115
refactor: split long-string for black
KyleKing ebf479d
refactor: use 'not in'
KyleKing aefe569
Revert "refactor: split long-string for black"
KyleKing 76b8783
fix: use source of plugin
KyleKing 402548b
refactor: move text inline and add resolution guidance
KyleKing e0787c7
refactor: consider restoring UserWarning
KyleKing f42d0b9
Update src/mdformat/_cli.py
hukkin 19a503e
Update tests/test_plugins.py
hukkin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DeprecationWarningto 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 ofmdformat-mkdocswouldn't have been visible to meI think a
UserWarningmight 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 examplesmdformat-tables=1.0.0won'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=PythonThere 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
DeprecationWarningis 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 withmdformat._cli.run.