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 --line-length option to format command #8363

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Add --line-length option to format command #8363

merged 2 commits into from
Nov 2, 2023

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Oct 30, 2023

Restores the --line-length option removed in #8131

Closes #8362
Closes #8352

We removed this option because we did not feel it made sense for the format CLI to expose a single configuration option but not others.

However, there are a few arguments in favor of restoring this option:

  1. It is the most asked for option (nobody has pointed out the other options are missing)
  2. It is also available via the ruff check CLI
  3. It is also available via the Black CLI
  4. There is no way for a user to configure the line width without a configuration file yet
  5. The line length is a unique formatter option; in that it is the most "restrictive" and not a toggle

While we intend to introduce override of general configuration options via the CLI, we have not built it yet. If we replace ruff check --line-length=... with a generic ruff check --setting line-length=... we'll need to manage a deprecation period anyway.

There remain some arguments against restoring this option:

  1. If/when we deprecate this option in favor of a --setting flag, more users will be affected
  2. Inconsistencies of the availability of configuration options via the CLI could be confusing
  3. Should this be --line-width to match our terminology for measuring enforced line length?

@zanieb zanieb added cli Related to the command-line interface formatter Related to the formatter labels Oct 30, 2023
@zanieb zanieb marked this pull request as ready for review October 30, 2023 18:09
@github-actions
Copy link
Contributor

PR Check Results

Ecosystem

✅ ecosystem check detected no linter changes.

✅ ecosystem check detected no format changes.

@T-256
Copy link
Contributor

T-256 commented Oct 30, 2023

  • If/when we deprecate this option in favor of a --setting flag, more users will be affected

IMO, this approach would be neater instead of --settings {option-name}=value:

...

Format configuration:
      --{option-name} <VALUE>  see formatter settings for possible options

      # (also all settings can be auto generated in this section)

Log levels:
  -v, --verbose  Enable verbose logging
  -q, --quiet    Print diagnostics, but nothing else
  -s, --silent   Disable all logging (but still exit with status code "1" upon detecting diagnostics)

Then --line-length no need to deprecate in future.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

I know we discussed this synchronously but I support this change for the reasons outlined in the summary. Thank you for writing them up.

@@ -409,6 +409,9 @@ pub struct FormatCommand {
force_exclude: bool,
#[clap(long, overrides_with("force_exclude"), hide = true)]
no_force_exclude: bool,
/// Set the line-length.
#[arg(long, help_heading = "Format configuration")]
pub line_length: Option<LineLength>,
Copy link
Member

Choose a reason for hiding this comment

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

This is actually hidden on ruff check... We could consider doing the same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh.. hm... I don't think we should hide things from the help menu if they're intended to be used?

Copy link
Member

Choose a reason for hiding this comment

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

Agree. We should either expose and document it or not add it.

It would be nice if we could add a message saying that we recommend using the configuration but it probably doesn't fit in nicely with the CLI documentation (not enough space, not in line with other CLI options)

@charliermarsh
Copy link
Member

Should this be --line-width to match our terminology for measuring enforced line length?

I believe we should change this if/when we change the terminology everywhere, and that for now, it's best to be consistent with the rest of the settings.

@zanieb
Copy link
Member Author

zanieb commented Oct 30, 2023

@T-256 I'm not sure that scales well to all of the settings. I've created a separate issue for discussion on that topic #8368

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I remain concerned about re-introducing the option but I believe removing it and now re-adding it at least served its purpose: We learned more about the use case where people want this option (e.g. pre-commit without a configuration file, VS Code).

My main concern is that we should encourage users to commit their formatter configuration to get consistent results across teams and interfaces (VS Code, CLI, ...). Adding this option is a step back in that regard.

More details:

  • It is also available via the ruff check CLI: But it is hidden and we discussed removing it.

    Adding a non-hidden line-length doesn't solve the inconsistency. It's differently inconsistent.

  • Using --line-length in VS code is rarely the right solution. The main advantage of a formatter is consistent formatting. Setting a user-specific --line-length doesn't achieve this goal (except if it is a workplace setting and committed). It further has the disadvantage that running ruff format on the CLI and in VS code produces different results. Understanding why VS code doesn't respect the line length (or configures code differently) may result in a lengthy debugging session.

  • Adding line-length unlocks the most common case, but it's still impossible to set all formatter configurations.

  • We're still not black compatible without exposing more options.

To sum it up. It unlocks some use cases but not all, nor does it fix any inconsistencies, it just changes them.

I fear we'll have a hard time justifying not adding other options. Adding them will, similarly, unlock the mentioned use cases for some users and fit the bill as nicely as --line-length. I'm okay with this moving forward but would prefer if we pursue alternatives (or justify why we won't) before exposing all formatter options.

@@ -409,6 +409,9 @@ pub struct FormatCommand {
force_exclude: bool,
#[clap(long, overrides_with("force_exclude"), hide = true)]
no_force_exclude: bool,
/// Set the line-length.
#[arg(long, help_heading = "Format configuration")]
pub line_length: Option<LineLength>,
Copy link
Member

Choose a reason for hiding this comment

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

Agree. We should either expose and document it or not add it.

It would be nice if we could add a message saying that we recommend using the configuration but it probably doesn't fit in nicely with the CLI documentation (not enough space, not in line with other CLI options)

@zanieb
Copy link
Member Author

zanieb commented Oct 31, 2023

Using --line-length in VS code is rarely the right solution.

While I agree with most of your points, many users use editors for Python files that do not belong to a real "project" and the usability of a formatter in this case is important.

@MichaReiser
Copy link
Member

While I agree with most of your points, many users use editors for Python files that do not belong to a real "project" and the usability of a formatter in this case is important.

I agree that this is a real use case and that we don't have any other solution for it yet. What I would prefer is something similar to Prettier's VS Code extension, allowing to configure a fallback for projects not using Ruff.

@zanieb
Copy link
Member Author

zanieb commented Nov 1, 2023

Oh interesting. I have a strong preference for that. However, I'm okay with merging this until we implement an alternative.

@charliermarsh I don't have the heart to do it though.

@danielhollas
Copy link

...Prettier's VS Code extension, allowing to configure a fallback for projects not using Ruff.

How would that that work for vi users?

@MichaReiser
Copy link
Member

...Prettier's VS Code extension, allowing to configure a fallback for projects not using Ruff.

How would that that work for vi users?

I'm not that familiar with Vim but it would be an lsp setting that you can set.

@zanieb zanieb merged commit 3a889f4 into main Nov 2, 2023
17 checks passed
@zanieb zanieb deleted the zanie/format-ll branch November 2, 2023 01:39
@inigohidalgo

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use line-length in command line for ruff format? Ruff formatter missing character limit argument
6 participants