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

Allow ellipsis default args in "trivial" non-stub functions #6665

Conversation

@Michael0x2a
Copy link
Collaborator

commented Apr 11, 2019

Resolves #6531

This diff modifies the checker to allow default arguments to be used in function definitions in non-stub files as long as the body of that function is either empty or contains just a raise NotImplementedError. (This is after ignoring docstrings.)

For rationale on why this is useful, see the issue linked above.

One note on function bodies containing only a raise Blah. I was worried that letting mypy ignore the ellipsis when the function raises any Exception would lead to unsafe behavior -- I didn't want mypy to start allowing code that looks like this:

def halt(self, reason: str = ...) -> NoReturn:
    raise MyCustomError("Fatal error: " + reason, self.line, self.current_context)

Whitelisting only NotImplementedError seemed like a good enough compromise -- it's unlikely users will use the arguments in non-trivial ways when raising that particular exception. It should also be easy enough to relax this restriction in the future if necessary.

Allow ellipsis default args in "trivial" non-stub functions
Resolves #6531

This diff modifies the checker to allow default arguments to
be used in function definitions in non-stub files as long as the
body of that function is either empty or contains just a
`raise NotImplementedError`. (This is after ignoring docstrings.)

For rationale on why this is useful, see the issue linked above.

One note on function bodies containing only a `raise Blah`. I was
worried that letting mypy ignore the ellipsis when the function
raises *any* Exception would lead to unsafe behavior -- I didn't
want mypy to start allowing code that looks like this:

    def halt(self, reason: str = ...) -> NoReturn:
        raise MyCustomError("Fatal error: " + reason, self.line, self.current_context)

Whitelisting only NotImplementedError seemed like a good enough
compromise -- it's unlikely users will use the arguments in non-trivial
ways when raising that particular exception. It should also be
easy enough to relax this restriction in the future if necessary.
@msullivan
Copy link
Collaborator

left a comment

Looks good.

This should make writing interfaces cleaner.

stmt = body[0]

# Bodies that just raise "NotImplementedError()" count as trivial.

This comment has been minimized.

Copy link
@msullivan

msullivan Apr 11, 2019

Collaborator

Add a doc string for this function, maybe by moving some of the inline comments to it?

@JukkaL

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

Maybe mention this somewhere in the mypy docs?

@ethanhs
Copy link
Collaborator

left a comment

I agree docs would be nice.

This will be very handy!

@ilevkivskyi
Copy link
Collaborator

left a comment

Thank you! Please don't forget to add the docs as suggested above before merging this

class Exception(BaseException): pass
class RuntimeError(Exception): pass
class NotImplementedError(RuntimeError): pass

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 12, 2019

Collaborator

No new line at the end of file.

Michael0x2a added some commits May 4, 2019

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

commented May 4, 2019

Sorry, returning to this only now. I added docs.

I did find writing them to be a bit difficult though: I wasn't sure exactly where to add the docs or how to best present the new info. I ended up shoving them into the page about stubs for now.

So, I'm a little leery of just landing this now. If it's not too much bother, would somebody mind doing one last sanity check over the docs I added?

@JelleZijlstra
Copy link
Collaborator

left a comment

Looks good to me (apart from one tiny suggestion).

It's a bit weird to discuss non-stub syntax in a page on stubs, but I'm not sure where else to put this.

docs/source/stubs.rst Outdated Show resolved Hide resolved
Update docs/source/stubs.rst
Co-Authored-By: Michael0x2a <michael.lee.0x2a@gmail.com>

@Michael0x2a Michael0x2a merged commit d08341e into python:master May 6, 2019

1 check passed

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

@Michael0x2a Michael0x2a deleted the Michael0x2a:allow-ellipsis-default-arguments-for-empty-bodies branch May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.