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

Fix stray type error in fs.py #10182

Merged
merged 4 commits into from Jun 27, 2020

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Jun 27, 2020

[ci skip-rust-tests]

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]
@stuhood
Copy link
Sponsor Member Author

stuhood commented Jun 27, 2020

Unclear why, but this is failing in an unrelated branch. I think that possibly due to undeclared dependencies, it only fails when run in particular ways? Not sure.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. I was hoping we wouldn't hit this. This was my exact fear with the thread yesterday at https://pantsbuild.slack.com/archives/CASMF8SJ1/p1593122804189300.

Indeed, this happens when you run on fs.py and internals:scheduler is not in the transitive closure. We have an undeclared dependency because of a dep cycle.

This PR will not fix things. Why? MyPy warns when you have unnecessary casts, so this cause errors when running lint ::, because internals:scheduler will now be in the closure and the import can be followed.

Instead, we need to do two things:

  1. Remove the conditional imports from interactive_runner.py and fs.py for now. They will not work robustly with v2 MyPy. Replace them with the type hint Any.
  2. Lean into your solution on dep cycles.

The takeaway is that Pants must support dep cycles for Python for MyPy to be usable in many codebases.

@stuhood
Copy link
Sponsor Member Author

stuhood commented Jun 27, 2020

  1. Lean into your solution on dep cycles.

That is in fact the unrelated PR, heh. On it.

This reverts commit 7fc122f.

[ci skip-rust-tests]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I would have expected you to need to remove from pants.engine.internals.scheduler import SchedulerSession, but if this passes, great.

[ci skip-rust-tests]
@stuhood stuhood force-pushed the stuhood/fix-stray-type-error branch from 49b4e9f to 3db3269 Compare June 27, 2020 00:50
@stuhood
Copy link
Sponsor Member Author

stuhood commented Jun 27, 2020

Thanks. I would have expected you to need to remove from pants.engine.internals.scheduler import SchedulerSession, but if this passes, great.

Yep: thank you. I read your comment more carefully the second time.

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]
@stuhood stuhood merged commit e5f6c9c into pantsbuild:master Jun 27, 2020
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.

None yet

2 participants