Rename *-length options to -width
#8010
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
MichaReiser
force-pushed
the
formatter-respect-tab-size
branch
from
366d2be to
a583805
Compare
MichaReiser
force-pushed
the
formatter-respect-tab-size
branch
from
a583805 to
f1b00ca
Compare
MichaReiser
changed the base branch from
formatter-respect-tab-size
to
options-deprecated
MichaReiser
force-pushed
the
rename-length-options
branch
from
ba61b69 to
0fee9d2
Compare
MichaReiser
added
configuration
MichaReiser
force-pushed
the
rename-length-options
branch
from
0fee9d2 to
90760b0
Compare
| /// Set the line-length. | ||
| #[arg(long, help_heading = "Rule configuration", hide = true)] | ||
| pub line_length: Option<LineLength>, |
There was a problem hiding this comment.
I removed this option from the formatter. I don't remember us explicitly deciding on exposing it and it was a hidden option anyway.
MichaReiser
force-pushed
the
options-deprecated
branch
from
f2ac348 to
03e903d
Compare
MichaReiser
force-pushed
the
rename-length-options
branch
from
90760b0 to
3be7279
Compare
MichaReiser
force-pushed
the
options-deprecated
branch
2 times, most recently
from
d601bfd to
f5ddc5a
Compare
MichaReiser
force-pushed
the
rename-length-options
branch
from
3be7279 to
3fb2088
Compare
MichaReiser
requested review from
charliermarsh and
zanieb
and removed request for
charliermarsh and
zanieb
MichaReiser
force-pushed
the
rename-length-options
branch
from
3fb2088 to
596a8ad
Compare
| "max-line-length" | "max_line_length" => match LineWidth::from_str(value) { | ||
| Ok(line_length) => options.line_width = Some(line_length), |
There was a problem hiding this comment.
I'm assuming that the line_length here wasn't renamed because it's coming from flake8?
There was a problem hiding this comment.
Yes, these are the options of the source configuration that gets rewritten to ruff.
crates/ruff_cli/src/lib.rs
Outdated
| #[allow(deprecated)] | ||
| if args.line_length.is_some() { | ||
| warn_user_once!("The option `--line-length` has been renamed to `--line-width` to emphasize that the limit is the display width of a line and not the number of characters. Use the option `--line-width` instead."); | ||
| } |
There was a problem hiding this comment.
Do you think it would be useful to specify when the option will be removed in the deprecation message? Like "This option will be removed in v0.3.0". It could also help us in finding the deprecated features which needs to be removed before bumping the version.
There was a problem hiding this comment.
I don't think I have enough historical data yet to make a good prediction. Like removing it in 0.3 would be too early if we happen to release it in the coming weeks.
| #[option( | ||
| default = "88", | ||
| value_type = "int", | ||
| example = r#" | ||
| # Allow lines to be as long as 120 characters. | ||
| # Allow lines to have a width up to 120. |
There was a problem hiding this comment.
nit: for consistency
| # Allow lines to have a width up to 120. | |
| # Allow lines to have a display width up to 120. |
There was a problem hiding this comment.
I think I might actually go the other way round and use the simpler term width everywhere.
|
@charliermarsh do you want to review this PR (it touches documentation ;)) or are you okay with me merging it. |
Signed-off-by: Micha Reiser <micha@reiser.io>
MichaReiser
force-pushed
the
rename-length-options
branch
from
596a8ad to
bcfb73b
Compare
|
@MichaReiser - Yes, I will review today! |
|
I support using width over length conceptually, and I support renaming these to reflect it, but I am wondering if we should wait until v0.2.0 to release this change (as hinted at in your PR summary). The main question for me is: what's the cost of keeping this PR open? How painful would it be to defer this change until v0.2.0? |
The cost isn't too high. I only expect conflicts with your documentation PR. The question is how far off is the 0.2.0 release? Does that mean we should also hold back with the other configuration changes (like renaming I think we should discuss this in the broader picture. Our versioning policy is clear that deprecating options is fine in a patch release. I don't mind holding off, but I think we should then consider changing our version policy as well. |
|
Good questions, though I don't think it's inconsistent with our versioning policy to hold off for a bit. I actually don't care if it's 0.2.0 or some later patch release -- I'm more focused on the cadence of the changes. There are still decisions to make within the confines of the policy. Like, under the policy, we could also just ship a minor release every day with breaking changes, and that would be fine too, but we obviously wouldn't want to do that :) Maybe I'm just over-analyzing the degree to which this will impact users. I do think a lot of users will be confused by |
That's fair. But how does this get solved by deferring the deprecating? This would be a justification to either not rename the option or support both options as aliases. Regarding a later patch release. I think that would be more confusing because the main reason is to align the options with the formatter. Shouldn't the changes be released with the formatter? The work we would need to redo is making sure that we incorporate the documentation improvements that I made but with the old terminology |
| @@ -65,7 +65,7 @@ pub(crate) fn format_imports( | |||
| block: &Block, | |||
| comments: Vec<Comment>, | |||
| locator: &Locator, | |||
| line_length: LineLength, | |||
| line_length: LineWidth, | |||
There was a problem hiding this comment.
Nit: should this be line_width?
| @@ -45,7 +45,7 @@ pub(crate) fn format_import_from( | |||
| import_from: &ImportFromData, | |||
| comments: &CommentSet, | |||
| aliases: &[(AliasData, CommentSet)], | |||
| line_length: LineLength, | |||
| line_length: LineWidth, | |||
There was a problem hiding this comment.
Nit: should this be line_width?
| @@ -138,7 +138,7 @@ pub(crate) fn format_imports( | |||
| #[allow(clippy::too_many_arguments, clippy::fn_params_excessive_bools)] | |||
| fn format_import_block( | |||
| block: ImportBlock, | |||
| line_length: LineLength, | |||
| line_length: LineWidth, | |||
There was a problem hiding this comment.
Nit: should this be line_width?
| /// limiting lines to 79 characters. By default, this rule enforces a limit | ||
| /// of 88 characters for compatibility with Black, though that limit is | ||
| /// configurable via the [`line-length`] setting. | ||
| /// limiting lines to 79 characters. By default, this rule enforces a [display-width](http://www.unicode.org/reports/tr11/#Overview) |
There was a problem hiding this comment.
Nit: remove dash in display-width? There's no dash in the linked source. (Also, do you mind changing the link to https?)
| @@ -64,7 +64,7 @@ impl Violation for DocLineTooLong { | |||
| #[derive_message_formats] | |||
| fn message(&self) -> String { | |||
| let DocLineTooLong(width, limit) = self; | |||
| format!("Doc line too long ({width} > {limit} characters)") | |||
| format!("Doc line too long ({width} > {limit} width)") | |||
There was a problem hiding this comment.
I don't know if width makes sense at the end here because it's not a unit. E.g., (80 > 79 width) seems incorrect. Perhaps just (80 > 79)? Could (80 > 79 characters) be arguably fine to keep as-is, since the length is 79 single-width characters?
There was a problem hiding this comment.
or "width 80 > max width 79" or "width 80 > 79" or "width of 80 > 79"
There was a problem hiding this comment.
Isn't it a unit? I first used Unicode widths but it felt lengthy.
I already found the existing one confusing because 88 > 80 characters compares unitless 88 with 80 characters. I like @zanieb's suggestion. Or I would write it as 88 > 80 [width]. I think that's the notation we used in university to denote the unit
There was a problem hiding this comment.
I think your response is correct, in that my concerns were fundamentally about this being confusing for users, and I was letting that leak into the timeline on which we shipped it out.
I do think that using "width" exposes some complexity to users that -- in the vast majority of cases -- they won't need to think about. The "number of characters" is an obvious concept, but the "width of a line" or the "width of a character" is not something that most people are familiar with. (The comment I made below about using (80 > 79 wide) vs. (80 > 79 characters) in the diagnostics is a symptom of this challenge.) This concern applies both to using width conceptually, and to using the "width" terminology.
But, having seen that Rustfmt and Prettier use the width terminology, I feel better about moving forward with it. And I trust your judgment that it's worth unifying the terminology with the underlying concept.
|
Replaced by #8150 |
Summary
This PR deprecates the
line-lengthandpycodestyle.max-doc-lengthoptions in favor of the newline-widthandpycodestyle.max-line-widthoptions. The motivation behind the rename is that the new options better emphasize the fact that the unit is the unicode display width and not the characters per line.I used this PR to change the diagnostic text for
E501andW505to use the termwidthinstead ofcharactersin the diagnostic messages.The old options act as aliases (including
ruff check --line-length) until we decide to remove them for good. Ruff prints a warning when it detects the usage of a deprecated configuration option. The deprecated options remain visible on the website but are marked as deprecated. The same is true when showing the options withruff config.This PR doesn't yet rename
tab-sizebecause I'm yet undecided whether it should betab-widthortab-spaces.Closes #7574
Closes #7573
Considerations
We may want to consider holding off on this rename, considering that
line-lengthis one of the most often used settings and the 0.1.0 was about stabilization. Deprecating the option is in line with our versioning policy (patch release is fine), but it may give the impression that Ruff isn't as stable as we claimed it to be with the 0.1 release.Test Plan
Newly added integration tests.