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 most formatter options to ruff.toml / pyproject.toml #7566

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Sep 21, 2023

Summary

This PR adds most formatter configuration options under a new [format] section.

Missing options

  • Tab size
  • Respecting exclude

Closes #7061
Closes #7570

Open Questions

  • Should we add line-ending as a global configuration option similar to line-width and tab-size to that Styleist could use it to. My main hesitation is that the formatter should default to LineEnding::Lf, but Styleist uses LineEnding::Auto (which makes sense for a linter).
  • Naming of LineEnding options: Are people familiar with Lf and CrLf or should we use Unix and Windows

Test Plan

  • I added a few integration tests.
  • Unfortunately, it seems insta-cmd normalizes line endings in snapshots (makes sense). So I used a hex viewer to verify that cr-lf results in \r\n line endings
  • I played around with the settings in the Playground

@MichaReiser
Copy link
Member Author

MichaReiser commented Sep 21, 2023

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 had to move the file because FiltePatternSet and FilePattern is defined in ruff_linter. We may want to introduce a new crate for these types eventually but moving the struct to ruff_workspace is sufficient for now because the formatter doesn't use it.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 21, 2023

CodSpeed Performance Report

Merging #7566 will degrade performances by 3.15%

Comparing format-options (7a13d26) with main (82978ac)

Summary

❌ 1 regressions
✅ 24 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main format-options Change
linter/default-rules[large/dataset.py] 91.1 ms 94.1 ms -3.15%

@@ -2381,11 +2390,141 @@ impl PyUpgradeOptions {
}
}

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
#[serde(untagged)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Using untagged allows us to support both the old SerializationFormat and Format group. The only downside is that serde's error messages aren't any useful because it only tells you that what you have is neither of the two.

@MichaReiser MichaReiser added configuration Related to settings and configuration formatter Related to the formatter labels Sep 21, 2023
@MichaReiser MichaReiser marked this pull request as ready for review September 21, 2023 10:26
@github-actions
Copy link
Contributor

github-actions bot commented Sep 21, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@konstin
Copy link
Member

konstin commented Sep 21, 2023

What about the following semantics for a global line-ending?

  • line-ending = "auto"

    • i'm using only the linter: line endings are determined from what the file already uses
    • i'm also using the formatter: line endings are changed to LF everywhere
  • line-ending = "native"/"lf"/"cr-lf"

    • i'm using only the linter: line endings are set to native/lf/cr-lf
    • i'm also using the formatter: line endings are changed to native/lf/cr-lf everywhere

By default, if a user only uses the linter, they get the usual conservative linter behaviour that tries to match what is already there, and won't notice the more aggresive formatter behaviour. If they use the formatter they get maximized consistency, since the formatter runs after the linter, user don't need to care about the preservation attempt in the linter. If a user has specific wishes, they can specify. Depending on their git and editor configuration, they also have the native option to keep the platform behaviour.

The formatter section documentation will link all global options that impact formatting style.

@MichaReiser
Copy link
Member Author

What about the following semantics for a global line-ending?

* line-ending = "auto"
  
  * i'm using only the linter: line endings are determined from what the file already uses
  * i'm also using the formatter: line endings are changed to LF everywhere

* line-ending = "native"/"lf"/"cr-lf"
  
  * i'm using only the linter: line endings are set to native/lf/cr-lf
  * i'm also using the formatter: line endings are changed to native/lf/cr-lf everywhere

By default, if a user only uses the linter, they get the usual conservative linter behaviour that tries to match what is already there, and won't notice the more aggresive formatter behaviour. If they use the formatter they get maximized consistency, since the formatter runs after the linter, user don't need to care about the preservation attempt in the linter. If a user has specific wishes, they can specify. Depending on their git and editor configuration, they also have the native option to keep the platform behaviour.

The formatter section documentation will link all global options that impact formatting style.

The challenge is that we don't know whether someone uses the formatter other than when they run ruff format or explicitly opt out of formatting using format.enabled = false.

I would also try to make settings as static as possible to avoid confusion. E.g. what happens if formatting is only used in some directories? Does Auto then have the same semantics for all files or depends on whether the file gets formatted?

@konstin
Copy link
Member

konstin commented Sep 21, 2023

The challenge is that we don't know whether someone uses the formatter other than when they run ruff format or explicitly opt out of formatting using format.enabled = false.

I would also try to make settings as static as possible to avoid confusion. E.g. what happens if formatting is only used in some directories? Does Auto then have the same semantics for all files or depends on whether the file gets formatted?

Assuming that if we run the formatter, we run it consistently after the linter, i don't think either of this is a problem: Files that get formatted get LF consistency, files that are not formatted have their line endings preserved by the linter. Put differently, i think the default should be the linter doesn't change any line endings (wrt to the file it's fixing), while the formatter changes all line endings.

@qarmin
Copy link

qarmin commented Sep 22, 2023

Default format configuration should be added here - https://github.com/astral-sh/ruff#configuration

@MichaReiser
Copy link
Member Author

The challenge is that we don't know whether someone uses the formatter other than when they run ruff format or explicitly opt out of formatting using format.enabled = false.
I would also try to make settings as static as possible to avoid confusion. E.g. what happens if formatting is only used in some directories? Does Auto then have the same semantics for all files or depends on whether the file gets formatted?

Assuming that if we run the formatter, we run it consistently after the linter, i don't think either of this is a problem: Files that get formatted get LF consistency, files that are not formatted have their line endings preserved by the linter. Put differently, i think the default should be the linter doesn't change any line endings (wrt to the file it's fixing), while the formatter changes all line endings.

I agree with this and I think this aligns with only having line-ending under format.

@MichaReiser
Copy link
Member Author

MichaReiser commented Sep 22, 2023

Default format configuration should be added here - astral-sh/ruff#configuration

Thanks for calling this out. I wasn't aware that we list the defaults in the README. I'm unsure yet what we should do about this, considering that the formatter options are preview only. I'll ask internally

@MichaReiser
Copy link
Member Author

Default format configuration should be added here - astral-sh/ruff#configuration

We discussed this internally. We wait with adding the section until reaching beta / stable.

@MichaReiser MichaReiser enabled auto-merge (squash) September 22, 2023 15:41
@MichaReiser MichaReiser merged commit 9d16e46 into main Sep 22, 2023
15 checks passed
@MichaReiser MichaReiser deleted the format-options branch September 22, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add formatter configuration options to ruff.toml Formatter: Add end of line configuration option
5 participants