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

ability to configure flags for a module from a comment within it #2938

Closed
dgoldstein0 opened this issue Mar 2, 2017 · 9 comments · Fixed by #6839
Closed

ability to configure flags for a module from a comment within it #2938

dgoldstein0 opened this issue Mar 2, 2017 · 9 comments · Fixed by #6839

Comments

@dgoldstein0
Copy link

I would like to be able to use some syntax like

# MYPY: strict

to turn on all the useful stricter-checking flags, like

--disallow-subclassing-any
--warn-return-any
--disallow-untyped-calls
--strict-optional

Not totally sure what each one does. Ideally we'd turn them all on globally, but ideally all our code would already be typed, and we don't live in that world. I'd prefer this to our current mypy.ini for a few reasons:

  • makes it really easy to turn on - no need to edit a separate file to add the flags
  • the setting follows the file around during renames and moves
  • it eliminates the need to know all the different flags

I suppose we'll still want to introduce new flags over time - to make stricter checking opt in so large codebases don't have to migrate all at once. So either mypy standardizes ways to refer to a few flags (e.g. the first 3 could be called --strict-any? or --no-implicit-any like typescript?) or we could allow configuring these aliases in mypy.ini. I think I'm a fan of the former - provides more standardization across codebases.

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 2, 2017

I can see the value of this, in part because this was my original idea for per-file configuration options. The options that can be set within a file should probably be named identically to either the command-line options or the config file options to avoid redundancy. I'd also spell the comment like this:

# mypy: option1
# mypy: option2

We could also support multiple options per line (# mypy: option1, option2).

For example:

# mypy: strict, no-strict-optional  # like --strict --no-strict-optional

(There already is --strict option that pretty does what was proposed. However, it's unclear if it should include things like --strict-boolean that may be a bit too opinionated.)

If we'd use the config file syntax, it would perhaps look like this (one option per line):

# mypy: strict=True
# mypy: strict_optional=False

I think that the options should be global to a file (i.e. no per-function options), and would be applied after config file options (but the config file would still be consulted as well).

@msullivan
Copy link
Collaborator

OK we need to do this, I think, because our internal mypy.ini is thousands of lines long and I want to start doing things that would make it longer.

My proposal is to allow both multiple options on a line and multiple lines. I prefer command line style, since writing =True is ugly and I think hyphens look better.
For multiple options on a line, I propose , (comma space) as the option separator, because just a space looks bad (without dashes in front of the arguments) and just a comma means we need some sort of escaping for things like always-true which takes a comma-delimited list.

So things would look like

# mypy: strict, no-strict-optional, always-true=WINDOWS,DEBUG

msullivan added a commit that referenced this issue May 15, 2019
This is essentially in preparation for a proof-of-concept
implementation of #2938 I am working on, which will want to
call into this code from build, and a build -> main dependency
seems wrong.
msullivan added a commit that referenced this issue May 15, 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.

Fixes #2938.

Thoughts?
@gvanrossum
Copy link
Member

While I initially gave this a thumbs-up, on second thought I am not comfortable with the choice of using comma+space as separator. I also agree that a space is less than ideal. How about one of the following?

  1. use ; as separator
  2. require quotes around values that need commas, e.g. always-true="WINDOWS,DEBUG"
  3. switch the separator for always-true to something else, e.g. /

I slightly favor (2).

msullivan added a commit that referenced this issue May 16, 2019
This is essentially in preparation for a proof-of-concept
implementation of #2938 I am working on, which will want to
call into this code from build, and a build -> main dependency
seems wrong.
msullivan added a commit that referenced this issue 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.

Fixes #2938.

Thoughts?
msullivan added a commit that referenced this issue 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.

Fixes #2938.

Thoughts?
@msullivan
Copy link
Collaborator

Yeah let's do option (2). Honestly I was mostly just trying to get out of writing the delimiter-quote-parsing code but (like every other part of this PR) it just wasn't actually that bad.

msullivan added a commit that referenced this issue 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.

Fixes #2938.

Thoughts?
@JukkaL
Copy link
Collaborator

JukkaL commented May 16, 2019

I like (2), since it seems to be the most general -- it allows both commas and semicolons as option values. I think that it also looks pretty nice in the common case. Maybe also allow escapes for double quotes and maybe things like newlines (just in case)?

@msullivan
Copy link
Collaborator

My inclination is to not bother with more complicated escaping syntax until we want to add options that could take advantage of it, but it wouldn't be hard to support also.

@JukkaL
Copy link
Collaborator

JukkaL commented May 17, 2019

The main potential use case for escaping that I can imagine is per-module flags to mypy plugins, which we don't support at the moment. So it seems fine to not support it for now. I couldn't even find an example where quoting the value would be useful, since none of the options where this is useful seem to be per-module options.

@gvanrossum
Copy link
Member

none of the options where this is useful seem to be per-module options

Well, always_true and always_false would require quoting and are per-module. I could totally see putting one of those at the top of a module if there's a flag variable that's important in that module only. E.g. a module that has debug = False and later some hacky debug code under if debug -- you may not care about type-checking that.

@gvanrossum
Copy link
Member

(Oh wait, never mind. You were talking about escaping quotes inside quoted strings. Yeah, I suppose we don't need that.)

msullivan added a commit that referenced this issue May 21, 2019
Implements line configuration using '# mypy: ' comments, following the
the discussion in #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 #2938.
PattenR pushed a commit to PattenR/mypy that referenced this issue Jun 23, 2019
This is essentially in preparation for a proof-of-concept
implementation of python#2938 I am working on, which will want to
call into this code from build, and a build -> main dependency
seems wrong.
PattenR pushed a commit to PattenR/mypy that referenced this issue 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants