Skip to content
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

add modules and packages keys to config #9891

Closed
wants to merge 4 commits into from

Conversation

trws
Copy link
Contributor

@trws trws commented Jan 8, 2021

Description

Fixes #9883

This adds configuration file support for specifying packages and
modules. I tried to preserve existing semantics between the config file
and command line. If the command line specifies anything, it overrides
the config file, if the config file contains both package/module and
files keys, the later key is ignored.

Test Plan

Added a new version of the package and module command line test that retrieves the values from the configuration file.

This adds configuration file support for specifying packages and
modules.  I tried to preserve existing semantics between the config file
and command line.  If the command line specifies anything, it overrides
the config file, if the config file contains both package/module and
files keys, the later key is ignored.

Also add tests and basic docs for the new keys.
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this fixes an inconsistency in the features supported by the command line vs. the config file.

Overall looks good, just a few nits. Thanks for also updating the docs!

@@ -278,6 +280,13 @@ def parse_section(prefix: str, template: Options,
if 'follow_imports' not in results:
results['follow_imports'] = 'error'
results[options_key] = v
if key in ('files', 'packages', 'modules'):
if all(('packages' in results.keys() or 'modules' in results.keys(),
results.get('files'))):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: This was a bit difficult to understand. I think it would better to use and instead of all(), and you can just use 'packages' in results, without using keys(), I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. I started with something that generated a list and tested it and didn't refactor it out as far as I should have. Fixed.

if key in ('files', 'packages', 'modules'):
if all(('packages' in results.keys() or 'modules' in results.keys(),
results.get('files'))):
print("May only specify one of: module/package, files, or command. Ignoring key.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does 'command' here refer to?

Please include the name of the key in the error message. Currently it may not be clear what is being ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, and I'll remove command, that's part of the corresponding CLI error message that I preserved but doesn't apply here.

Addresses comments, adds new test to ensure that the key order is
honored as documented and error message presents keys correctly.
@trws
Copy link
Contributor Author

trws commented Jan 25, 2021

Anything further here? I'd be happy to make other tweaks if necessary.

@97littleleaf11 97littleleaf11 added this to Review in progress in mypy Nov 25, 2021
@97littleleaf11 97littleleaf11 moved this from Review in progress to Waiting for review in mypy Nov 26, 2021
@hauntsaninja
Copy link
Collaborator

Thanks for working on this, it looks like this was superseded by #13404

mypy automation moved this from Waiting for review to Done Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
mypy
Done
Development

Successfully merging this pull request may close these issues.

Add packages and modules keys to config file format
4 participants