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

Support '# mypy: ' comments for inline configuration #6839

Merged
merged 5 commits into from
May 21, 2019

Conversation

msullivan
Copy link
Collaborator

@msullivan msullivan commented May 16, 2019

Implements line configuration using '# mypy: ' comments, following the
blueprint I proposed in #2938.

It currently finds them just using a regex which means it is possible to pick
up a directive spuriously in a string literal or something but honestly I am
just not worried about that in practice.

Examples of what it looks like in the tests.
Still needs documentation.

Fixes #2938.

Thoughts?

(This is stacked on #6838 so can't be merged yet)

@msullivan msullivan changed the base branch from per-module-flags to master May 16, 2019 05:46
Implements line configuration using '# mypy: ' comments, following the
blueprint I proposed in #2938.

It currently finds them just using a regex which means it is possible to pick
up a directive spuriously in a string literal or something but honestly I am
just not worried about that in practice.

Examples of what it looks like in the tests.

Fixes #2938.

Thoughts?
@JukkaL
Copy link
Collaborator

JukkaL commented May 16, 2019

General idea looks solid to me. Probably worth adding some tests for invalid # mypy: ... comments. And I guess we should make sure that # mypy: skip-file either generates an error or does something reasonable. Maybe it should skip processing of the file?

I didn't do a proper review (yet).

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

This is great! Two nits; and one thing that saddens me a little, but I'll leave it up to you to decide whether to do anything about the error line number or not.

mypy/build.py Outdated Show resolved Hide resolved
mypy/config_parser.py Show resolved Hide resolved
mypy/util.py Outdated Show resolved Hide resolved
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks! This will be a great usabilitiy boost. I just have two random comments.

mypy/build.py Outdated Show resolved Hide resolved
[case testInlineError1]
# mypy: invalid-whatever
[out]
main: error: Unrecognized option: invalid_whatever = True
Copy link
Member

Choose a reason for hiding this comment

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

I would add more tests for the cases when user did something wrong:

  • what if there is extra spaces around =?
  • what if ; used as a separator?
  • what if a user forgot to quote a flag value that has commas?
    etc.

@msullivan msullivan changed the title Initial implementation of inline configuration Support '# mypy: ' comments for inline configuration May 21, 2019
@msullivan
Copy link
Collaborator Author

I am happy with how this all looks now.

Currently we produce an error on a skip-file directive. We could make it actually skip in a follow-up PR.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Looking forward to this!

@msullivan msullivan merged commit 67d0902 into master May 21, 2019
@msullivan msullivan deleted the per-module-flags-actually branch May 21, 2019 19:33
PattenR pushed a commit to PattenR/mypy that referenced this pull request Jun 23, 2019
Implements line configuration using '# mypy: ' comments, following the
the discussion in python#2938.

It currently finds them in a pretty primitive manner which means it is possible
to pick up a directive spuriously in a string literal or something but honestly I am
just not worried about that in practice.

Documentation to come in a follow-up PR.

Fixes python#2938.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ability to configure flags for a module from a comment within it
4 participants