Fix TOML boolean false values for store_true options#10903
Fix TOML boolean false values for store_true options#10903marwinsteiner wants to merge 2 commits intopylint-dev:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (86.95%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10903 +/- ##
==========================================
+ Coverage 96.04% 96.15% +0.11%
==========================================
Files 177 178 +1
Lines 19628 19641 +13
==========================================
+ Hits 18851 18886 +35
+ Misses 777 755 -22
🚀 New features to boost your workflow:
|
|
One CI failure, but it looks like a false positive to me: ************* Module pylint.config.config_file_parser
Notice: Wrong spelling of a word 'linter's' in a comment:
# Build a set of store_true option names from the linter's arg parser
^^^^^^^^
Did you mean: ''liner's' or 'liter's''?
Notice: Wrong spelling of a word 'natively' in a docstring:
TOML files can express boolean values natively (true/false). When these
^^^^^^^^
Did you mean: ''naively' or 'negatively''?
Notice: Consider using set for membership test |
DanielNoord
left a comment
There was a problem hiding this comment.
Why not do this in parse_toml_file?
| def _fix_store_true_options(self, options: list[str]) -> list[str]: | ||
| """Fix boolean values for store_true argparse actions. | ||
|
|
||
| TOML files can express boolean values natively (true/false). When these |
There was a problem hiding this comment.
You can add it to our spelling dictionary in the root of the repository
There was a problem hiding this comment.
You can add it to our spelling dictionary in the root of the repository
Done. Moved the logic into parse_toml_file and added the spelling words. Thanks for the review!
be11ef4 to
bc4faf4
Compare
for more information, see https://pre-commit.ci
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 733f62b |
| @staticmethod | ||
| def parse_toml_file(file_path: Path) -> PylintConfigFileData: | ||
| def parse_toml_file( | ||
| file_path: Path, store_true_options: set[str] | None = None |
There was a problem hiding this comment.
Why is this optional?
| for opt, values in sections_values.items(): | ||
| if isinstance(values, dict): | ||
| for config, value in values.items(): | ||
| value = _parse_rich_type_value(value) |
There was a problem hiding this comment.
There is a lot of duplication. Have you explored if we can do this in _parse_rich_type_value instead?
Summary
store_trueconfig options (likeexit-zero) in TOML files not respectingfalsevaluesexit-zero = falseis set in TOML, pylint previously treated it the same asexit-zero = truebecause argparsestore_trueactions activate from flag presence alone, ignoring the trailing"False"string value_fix_store_true_optionsto_ConfigurationFileParserthat introspects the linter's argparse actions to correctly handle boolean values forstore_trueoptions only (notyn-type options like--reports)How it works
After
_RawConfParserconverts TOML values to["--flag", "True"/"False"]string pairs,_fix_store_true_optionschecks each option against the linter's registered_StoreTrueActionactions:--exit-zero) without the trailing value stringThis distinction is important because other boolean-like options (e.g.
--reports) use argparse'syntype and do expect a value argument.Test plan
exit-zero = falsein TOML (verifiesexit_zeroisfalse)exit-zero = truein TOML (verifiesexit_zeroistrue)Closes #8460