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

Add test and basic implementation for formatter preview mode #8044

Merged
merged 6 commits into from
Oct 26, 2023

Conversation

konstin
Copy link
Member

@konstin konstin commented Oct 18, 2023

Summary Prepare for the black preview style becoming the black stable style at the end of the year.

This adds a new test file to compare stable and preview on some relevant preview options in black, and makes format_dev understand the black preview flag. I've added poetry as a project that uses preview.

I've implemented one specific deviation (collapsing of stub implementation in non-stub files) which showed up in poetry for testing. This also improves poetry compatibility from 0.99891 to 0.99919.

Fixes #7440

New compatibility stats:

project similarity index total files changed files
cpython 0.75803 1799 1647
django 0.99983 2772 35
home-assistant 0.99953 10596 189
poetry 0.99919 317 12
transformers 0.99963 2657 332
twine 1.00000 33 0
typeshed 0.99978 3669 20
warehouse 0.99969 654 15
zulip 0.99970 1459 22

@konstin konstin added the formatter Related to the formatter label Oct 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@@ -1,13 +1,45 @@
---
source: crates/ruff_python_formatter/tests/fixtures.rs
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/assign_breaking.py
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/preview.py
Copy link
Member

Choose a reason for hiding this comment

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

Should we instead run every test under preview and non-preview?

Copy link
Member

Choose a reason for hiding this comment

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

If the cost is not too high that seems ideal

Copy link
Member Author

Choose a reason for hiding this comment

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

hyperfine "cargo test -p ruff_python_formatter" goes from 500ms to 580ms on my machine, acceptable for me.

Having both versions makes it hard to see what actually changed given our large snapshots, so i opted for instead adding the diff as separate code block if there is one. I also changed all non-function/class tests to use pass instead as real code generally does, which i split out to #8049 to keep the diff readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to keep preview.py as-is though, it's a useful tool for tracking progress and comparing to black (i can diff both snapshots with what the black playground does)

@konstin konstin changed the base branch from main to pass-over-ellipsis-for-preview October 18, 2023 16:59
@konstin
Copy link
Member Author

konstin commented Oct 18, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

)
.unwrap();
// We want to capture the differences in the preview style in our fixtures
let options_preview = options.with_preview(PreviewMode::Enabled);
Copy link
Member

Choose a reason for hiding this comment

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

Should we skip this if options.preview == Enabled when using a test specific options file?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the no-options-file branch; i think it's better to not mix them to avoid combinatorial explosion

@@ -996,4 +996,167 @@ def default_arg_comments2( #
```


## Preview changes
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how noisy this gets once we start having many preview style rules. But let's see how it goes.

Base automatically changed from pass-over-ellipsis-for-preview to main October 19, 2023 09:11
konstin added a commit that referenced this pull request Oct 19, 2023
Split out of #8044: In preview style, ellipsis are also collapsed in
non-stub files. This should only affect function/class contexts since
for other statements stub are generally not used. I've updated our tests
to use `pass` instead to reflect this, which makes tracking the preview
style changes much easier.
@charliermarsh
Copy link
Member

@konstin - Is this good to merge?

@konstin
Copy link
Member Author

konstin commented Oct 26, 2023

yep

@konstin konstin enabled auto-merge (squash) October 26, 2023 15:23
@konstin konstin merged commit 317d3dd into main Oct 26, 2023
16 checks passed
@konstin konstin deleted the formatter-preview branch October 26, 2023 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatter incompatibility: ellipses are not inlined in .py files
4 participants