Skip to content

Make bare None a partial type again #3755

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

Closed
wants to merge 1 commit into from

Conversation

ddfisher
Copy link
Collaborator

Fixes #3741. Only merge after reading discussion in #3741 and deciding this is the right thing.

@gvanrossum
Copy link
Member

This gives lots of new errors in our internal codebase, so I think this PR is dead on arrival.

@ilevkivskyi
Copy link
Member

I think a better idea might be to only do this for self.attr = None assignments. IIUC this is how PyCharm does this.

@gvanrossum
Copy link
Member

I wish we could do partial types across methods (and include class variables too).

@ddfisher
Copy link
Collaborator Author

Yeah. See my comment in 3741. I'm not actually convinced this is the right idea, but I put it up in case you thought it was and wanted to merge it for 0.521.

I spot-checked some of the internal errors, and a decent number were from self.attr = None assignments, so I'm not sure that would help.

Is this a real user-confusion issue, or is it only theoretical at this point?

@ilevkivskyi
Copy link
Member

Is this a real user-confusion issue, or is it only theoretical at this point?

FWIW, this came form @vlasovskikh on gitter.

@vlasovskikh
Copy link
Member

vlasovskikh commented Jul 24, 2017

@ddfisher I haven't encountered anyone besides me who complains about this problem. In PyCharm we infer Any for such fields instead of None since we don't want to force users into using type hints just to make the warnings go away. But since you already have such warnings for empty lists, it makes sense to handle the None-initialized fields in the same way. It is highly unlikely that somebody wants an attribute of type None, so inferring None makes little sense.

@gvanrossum
Copy link
Member

@JukkaL, what do you think?

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 24, 2017

If we change this, it will potentially affect a lot of existing code, so it looks we can't really include the change in the 0.521 release. Let's reconsider this for 0.530 after 0.521 has been released. Just doing this for self.attr = None is a potential compromise, but again we can't include this in 0.521 because it could start generating errors for code that's clean in 0.520.

@ddfisher
Copy link
Collaborator Author

ddfisher commented Sep 7, 2017

I think we've basically decided we're not going to do this.

@ddfisher ddfisher closed this Sep 7, 2017
@gvanrossum gvanrossum deleted the change-none-partial-behavior branch September 29, 2017 21:34
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.

5 participants