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

Refactor V2 Black implementation #8671

Merged
merged 9 commits into from Nov 21, 2019

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Nov 21, 2019

We will soon be adding a V2 implementation for isort. This PR is prework to make the Black code a bit more generic so that we can have Black and isort coincide as two rules for fmt-v2 and lint-v2.

  • Splits out python_fmt into rules/black.py and targets/formattable_python_target
    • This will allow the new isort logic to live in rules/isort.py and reuse the FormattablePythonTarget abstraction created originally for Black
  • Move _generate_black_pex_args() and _generate_black_request() onto methods defined on BlackInput/BlackSetup to mirror our Pex dataclass
  • Rename BlackInput to BlackSetup and add docstring explaining why we use this abstraction.
  • Use the new async await style for rules
  • Change --black-config to not use fingerprint=True, which is irrelevant in a V2 world.

…on_target

This will allow us to add rules/isort.py
@Eric-Arellano Eric-Arellano changed the title Refactor V2 black implementation Refactor V2 Black implementation Nov 21, 2019
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

So, there is potentially a larger concern with installing multiple auto-formatters this way. Which is that we haven't designed how to have multiple formatters depend on one another's output without knowing about eachother.

src/python/pants/backend/python/rules/black.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

So, there is potentially a larger concern with installing multiple auto-formatters this way. Which is that we haven't designed how to have multiple formatters depend on one another's output without knowing about eachother.

Oh! Interesting point! I don't think that should block this PR, but I agree that we'll need to design this interaction.

src/python/pants/backend/python/rules/black.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pierrechevalier83 pierrechevalier83 left a comment

Choose a reason for hiding this comment

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

I like these changes a lot. Thanks for this. Left one comment about the one change that makes me a tad worried that we may be breaking functionality, but if it works as expected, I'm good with this.

src/python/pants/backend/python/rules/black.py Outdated Show resolved Hide resolved
Copy link
Contributor

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Looks good (subject to @pierrechevalier83 concerns, which hopefully we can resolve one way or another by adding a test :)) Thanks!

src/python/pants/backend/python/rules/black.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/rules/black.py Outdated Show resolved Hide resolved
@Eric-Arellano Eric-Arellano merged commit fc3fbd5 into pantsbuild:master Nov 21, 2019
@Eric-Arellano Eric-Arellano deleted the refactor-black branch November 21, 2019 23:30
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

5 participants