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 prior_formatter_result and its usage #14987

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

thejcannon
Copy link
Member

The main change here is renaming prior_formatter_result to be snapshot, and removing | None from its type.

Then call-sites must be refactored to remove the if None conditional since its never None. Which then leads to removing the source_files Get.
Also, now original_snapshot in most formatter's Setup is superfluous. So it can be replaced by the Process it contains. More refactoring.
Lastly, I added FmtResult.create since the request/result contain most of the requisite info.

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon added the category:internal CI, fixes for not-yet-released features, etc. label Apr 3, 2022
@@ -49,7 +48,6 @@ class GoogleJavaFormatToolLockfileSentinel(GenerateToolLockfileSentinel):
@dataclass(frozen=True)
class Setup:
Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to elide this class with the JvmProcess directly causes a graph error :|

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the trouble there. If you want to open a ticket about that linked to #11269 feel free.

@thejcannon thejcannon changed the title [internal] Refactor prior_formatter_result and it's usage [internal] Refactor prior_formatter_result and its usage Apr 4, 2022
Copy link
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.

Thanks!

@@ -49,7 +48,6 @@ class GoogleJavaFormatToolLockfileSentinel(GenerateToolLockfileSentinel):
@dataclass(frozen=True)
class Setup:
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the trouble there. If you want to open a ticket about that linked to #11269 feel free.

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.

Yay! +1 to stu's suggestion, several files need it

this is plugin API change, not internal

@thejcannon thejcannon added category:plugin api change and removed category:internal CI, fixes for not-yet-released features, etc. labels Apr 4, 2022
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon changed the title [internal] Refactor prior_formatter_result and its usage Refactor prior_formatter_result and its usage Apr 4, 2022
@thejcannon thejcannon merged commit efb4ade into pantsbuild:main Apr 4, 2022
@thejcannon thejcannon deleted the betterprior branch April 4, 2022 20:46
stuhood added a commit to stuhood/pants that referenced this pull request Jun 28, 2022
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants