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 flag to output error if a typed function is used with an untyped decorator #3555

Merged
merged 9 commits into from Aug 3, 2017

Conversation

Projects
None yet
3 participants
@ilinum
Collaborator

ilinum commented Jun 15, 2017

No description provided.

ilinum added some commits Jun 15, 2017

Output error if a typed function is used with an untyped decorator
and the --disallow-untyped-defs flag is enabled
@ddfisher

Before merging, can you run this against our internal codebases and make sure that there aren't too many errors and that the errors seem like they really should be fixed?

Show outdated Hide outdated mypy/checker.py
Show outdated Hide outdated mypy/checker.py
@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jul 5, 2017

Member

The main concern is that this will be seen as a regression by people who have carefully annotated (part of) their codebase but are using untyped decorators (maybe from another part of their codebase). On the one hand these people have a problem (unannotated code persists, and the call sites may have further errors), on the other hand they may not have the bandwidth to solve it quickly, and until it's solved they can no longer use the --disallow-untyped-defs flag at all to require that all functions to be annotated. Which is why I would prefer this to be a separate flag (e.g. --disallow-untyped-decorators).

Member

gvanrossum commented Jul 5, 2017

The main concern is that this will be seen as a regression by people who have carefully annotated (part of) their codebase but are using untyped decorators (maybe from another part of their codebase). On the one hand these people have a problem (unannotated code persists, and the call sites may have further errors), on the other hand they may not have the bandwidth to solve it quickly, and until it's solved they can no longer use the --disallow-untyped-defs flag at all to require that all functions to be annotated. Which is why I would prefer this to be a separate flag (e.g. --disallow-untyped-decorators).

@ilinum

This comment has been minimized.

Show comment
Hide comment
@ilinum

ilinum Jul 6, 2017

Collaborator

After thinking about this some more, I agree with you, this change would discourage use of --disallow-untyped-defs, which is something we definitely we want to avoid.

I think it would be reasonable to have a new flag --disallow-untyped-decorators.

Just a note that there is --disallow-any=decorator (460a13b) that disallows functions that have Any in their signature after decorator transformation.

Collaborator

ilinum commented Jul 6, 2017

After thinking about this some more, I agree with you, this change would discourage use of --disallow-untyped-defs, which is something we definitely we want to avoid.

I think it would be reasonable to have a new flag --disallow-untyped-decorators.

Just a note that there is --disallow-any=decorator (460a13b) that disallows functions that have Any in their signature after decorator transformation.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jul 6, 2017

Member
Member

gvanrossum commented Jul 6, 2017

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jul 12, 2017

Collaborator

(The flag is actually --disallow-any=decorated, which is definitely better than --disallow-any=decorator.)

Collaborator

ddfisher commented Jul 12, 2017

(The flag is actually --disallow-any=decorated, which is definitely better than --disallow-any=decorator.)

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jul 12, 2017

Collaborator

I think a new flag is the right way forward here. @ilinum can you make that change?

Collaborator

ddfisher commented Jul 12, 2017

I think a new flag is the right way forward here. @ilinum can you make that change?

@ilinum ilinum changed the title from Output error if a typed function is used with an untyped decorator and the `--disallow-untyped-defs` flag is enabled to Flag to output error if a typed function is used with an untyped decorator Jul 20, 2017

@ddfisher ddfisher changed the title from Flag to output error if a typed function is used with an untyped decorator to Add flag to output error if a typed function is used with an untyped decorator Jul 20, 2017

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jul 20, 2017

Collaborator

(Minor note: PR/commit names should be actions.)

Collaborator

ddfisher commented Jul 20, 2017

(Minor note: PR/commit names should be actions.)

@ddfisher

Some functionality and testing changes needed.

Show outdated Hide outdated mypy/checker.py
Show outdated Hide outdated test-data/unit/check-flags.test
Show outdated Hide outdated test-data/unit/cmdline.test
@d # no error
def f(): pass

This comment has been minimized.

@ddfisher

ddfisher Jul 20, 2017

Collaborator

Please add tests for partially typed functions.

@ddfisher

ddfisher Jul 20, 2017

Collaborator

Please add tests for partially typed functions.

This comment has been minimized.

@ilinum

ilinum Aug 3, 2017

Collaborator

Done!

@ilinum

ilinum Aug 3, 2017

Collaborator

Done!

@ilinum

This comment has been minimized.

Show comment
Hide comment
@ilinum

ilinum Aug 3, 2017

Collaborator

Okay, code review feedback should be addressed now!

Collaborator

ilinum commented Aug 3, 2017

Okay, code review feedback should be addressed now!

@ddfisher

LGTM! Tiny tiny nit -- feel free to merge if you disagree, otherwise fix and then merge.

Show outdated Hide outdated mypy/checker.py

@ilinum ilinum merged commit 7d7c71d into python:master Aug 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilinum ilinum deleted the ilinum:disallow-untyped-defs-decorated branch Aug 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment