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

Refactor: Move semantic analyzer pass 3 to a new module #4099

Merged
merged 1 commit into from Oct 12, 2017

Conversation

Projects
None yet
2 participants
@JukkaL
Collaborator

JukkaL commented Oct 12, 2017

Just move things around, tweak inter-module references and add
necessary imports.

The third pass still directly accesses the second pass so there is
more work to do.

Refactor: Move semantic analyzer pass 3 to a new module
Just move things around, tweak inter-module references and add
necessary imports.

The third pass still directly accesses the second pass so there is
more work to do.
@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 12, 2017

Collaborator

I'm going to merge this soon if nobody says anything so that I can move on with follow-up refactorings. This is all straightforward.

Collaborator

JukkaL commented Oct 12, 2017

I'm going to merge this soon if nobody says anything so that I can move on with follow-up refactorings. This is all straightforward.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 12, 2017

Collaborator
Collaborator

ilevkivskyi commented Oct 12, 2017

@@ -11,3 +11,6 @@ warn_unused_ignores = True
# historical exception
[mypy-mypy.semanal]
strict_optional = False
[mypy-mypy.semanal_pass3]
strict_optional = False

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 12, 2017

Collaborator

This PR will cause several conflicts in last strict optional PR #4070

They are all however more or less straightforward to fix, so I am OK with merging this first.

By the way, if you are not happy with the if var in List[int]: ... narrowing that PR #4070 introduces, then I can limit it only to narrowing optional to non-optional, which is safe and consistent with existing narrowing optional to non-optional by ==. And then later we can consider the more general approach like #4072

@ilevkivskyi

ilevkivskyi Oct 12, 2017

Collaborator

This PR will cause several conflicts in last strict optional PR #4070

They are all however more or less straightforward to fix, so I am OK with merging this first.

By the way, if you are not happy with the if var in List[int]: ... narrowing that PR #4070 introduces, then I can limit it only to narrowing optional to non-optional, which is safe and consistent with existing narrowing optional to non-optional by ==. And then later we can consider the more general approach like #4072

This comment has been minimized.

@JukkaL

JukkaL Oct 12, 2017

Collaborator

Thanks! Sorry about the conflicts -- I'll try not to do any complex refactorings until the strict optional PR has been merged.

Just doing the narrowing for optional sounds like a good idea. Non-optional cases seem very rare and they are potentially unsafe. If you can update #4072 soon I should be able to merge it (and #4070) before some further refactorings land.

@JukkaL

JukkaL Oct 12, 2017

Collaborator

Thanks! Sorry about the conflicts -- I'll try not to do any complex refactorings until the strict optional PR has been merged.

Just doing the narrowing for optional sounds like a good idea. Non-optional cases seem very rare and they are potentially unsafe. If you can update #4072 soon I should be able to merge it (and #4070) before some further refactorings land.

@JukkaL JukkaL merged commit 5bdf0bf into master Oct 12, 2017

4 checks passed

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

@gvanrossum gvanrossum deleted the refactor-semanal-pass3 branch Oct 13, 2017

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