feat(rome_service): `--apply-suggested` argument to `rome check` #2952

ematipico · 2022-07-28T11:22:26Z

Summary

`--apply-suggested` argument

This PR adds a new argument --apply-suggested to the check.

As opposed to --apply, which only applies safe fixes, this argument tells to the workspace to apply fix actions that are marked with suggested fixes, marked with Applicability::MaybeIncorrect and safe fixes.

This argument is temporary and it will be replaced with another argument called --review, which will allow users to interactively choose the safe fixes.

Settings passed to format requests

Until now we were passing the IndentStyle coming from the editor, which would then override the one coming from the configuration. The PR removes the need to sending the indentation coming from the LSP. We now have a configuration file and Rome should rely on what the user specified there.

Messaging of the `rome check --apply` command

When using --apply, the workspace now tracks the fixes marked as suggested, and at the end it will present a message with the amount of suggested fixes that were not applied, and it will suggest the new argument added in this PR

Test Plan

I added new tests

github-actions · 2022-07-28T11:26:01Z

Playground for commit 3fe66c9

cloudflare-workers-and-pages · 2022-07-28T11:28:54Z

Deploying with Cloudflare Pages

Latest commit:	`7706549`
Status:	✅ Deploy successful!
Preview URL:	https://1a46ee69.tools-8rn.pages.dev
Branch Preview URL:	https://fix-check-apply-command.tools-8rn.pages.dev

View logs

crates/rome_cli/src/traversal.rs

leops · 2022-07-29T14:50:19Z

crates/rome_service/src/settings.rs

 pub(crate) struct SettingsHandle<'a, E> {
    inner: RwLockReadGuard<'a, WorkspaceSettings>,
    /// Additional per-request state injected by the editor
+    #[allow(dead_code)]


If this field is now unused let's remove it

Are you sure about it? It's definitely something that we will need in the future if we decided to have options in the editors.

At least at the moment I don't really see any use case for being able to provide configuration override into the workspace on a per-request basis

MichaReiser · 2022-07-29T15:06:17Z

Until now we were passing the IndentStyle coming from the editor, which would then override the one coming from the configuration. The PR removes the need to sending the indentation coming from the LSP. We now have a configuration file and Rome should rely on what the user specified there.

What if there's no configuration in place? Shouldn't Rome then use the editor defaults?

MichaReiser

What do you think of changing the --apply-suggested argument to --apply=suggested?

crates/rome_cli/src/commands/check.rs

ematipico · 2022-08-02T07:01:31Z

What do you think of changing the --apply-suggested argument to --apply=suggested?

From the description:

This argument is temporary and it will be replaced with another argument called --review, which will allow users to interactively choose the safe fixes.

--review comes from Rome classic, and it was the "way to go" to apply suggested fixes. So I am not sure if it's safe to change the --apply argument behaviour, to change it back when we will introduce --review

ematipico · 2022-08-02T07:45:46Z

Until now we were passing the IndentStyle coming from the editor, which would then override the one coming from the configuration. The PR removes the need to sending the indentation coming from the LSP. We now have a configuration file and Rome should rely on what the user specified there.

What if there's no configuration in place? Shouldn't Rome then use the editor defaults?

I think we should not use editor's configuration, because they would go in conflict with commands like rome ci. If we can't keep the configuration stable across environments, we risk to create errors. Example: if spaces are set in the editor, we save and commit, then when we run rome ci in a CI environment, tabs will be used (Rome's default) and it will emit errors.

I am not saying we should remove it, but the ergonomics will be harder. What do you think?

leops · 2022-08-02T12:11:12Z

Until now we were passing the IndentStyle coming from the editor, which would then override the one coming from the configuration. The PR removes the need to sending the indentation coming from the LSP. We now have a configuration file and Rome should rely on what the user specified there.

What if there's no configuration in place? Shouldn't Rome then use the editor defaults?

I think we should not use editor's configuration, because they would go in conflict with commands like rome ci. If we can't keep the configuration stable across environments, we risk to create errors. Example: if spaces are set in the editor, we save and commit, then when we run rome ci in a CI environment, tabs will be used (Rome's default) and it will emit errors.

I am not saying we should remove it, but the ergonomics will be harder. What do you think?

I think that's also what Prettier is doing, when no configuration file is present it doesn't read its settings from the incoming formatting request, it just uses the default configuration of 2 spaces for indentation

…e#2952)

ematipico temporarily deployed to aws July 28, 2022 11:22 Inactive

ematipico force-pushed the fix/check-apply-command branch from 46a8ae7 to eae2c77 Compare July 28, 2022 12:54

ematipico temporarily deployed to aws July 28, 2022 12:54 Inactive

ematipico changed the title ~~feat(rome_service): apply formatting when running check --apply~~ feat(rome_service): new --apply-suggested argument to rome check Jul 29, 2022

ematipico force-pushed the fix/check-apply-command branch 2 times, most recently from 71da2ac to 167156f Compare July 29, 2022 10:59

ematipico temporarily deployed to aws July 29, 2022 10:59 Inactive

ematipico temporarily deployed to aws July 29, 2022 11:11 Inactive

ematipico commented Jul 29, 2022

View reviewed changes

crates/rome_cli/src/traversal.rs Outdated Show resolved Hide resolved

ematipico marked this pull request as ready for review July 29, 2022 11:12

ematipico requested review from leops and xunilrj as code owners July 29, 2022 11:12

ematipico requested a review from a team July 29, 2022 11:12

leops reviewed Jul 29, 2022

View reviewed changes

MichaReiser reviewed Jul 29, 2022

View reviewed changes

crates/rome_cli/src/commands/check.rs Outdated Show resolved Hide resolved

ematipico requested review from leops and MichaReiser August 2, 2022 11:11

ematipico force-pushed the fix/check-apply-command branch from 1bfcea9 to e78d1a7 Compare August 2, 2022 11:14

ematipico temporarily deployed to aws August 2, 2022 11:14 Inactive

ematipico temporarily deployed to aws August 2, 2022 11:18 Inactive

ematipico force-pushed the fix/check-apply-command branch from 6ad137b to 396f98c Compare August 2, 2022 12:36

ematipico temporarily deployed to aws August 2, 2022 12:37 Inactive

ematipico added this to the 0.9.0 milestone Aug 3, 2022

ematipico added the A-Linter Area: linter label Aug 3, 2022

ematipico added the A-CLI Area: CLI label Aug 3, 2022

ematipico added 9 commits August 3, 2022 11:19

feat(rome_service): apply formatting when running check --apply

5b742f7

chore: remove rule from being recommended

68e92f0

chore: remove indent style from requests

07c1e66

chore: apply suggested fixes

67ae58a

chore: clippy and tests

aa7aace

chore: apply suggestions

f50c702

chore: clippy

adf263a

chore: remove unused code

b5015e2

chore: rebase

7706549

ematipico force-pushed the fix/check-apply-command branch from 396f98c to 7706549 Compare August 3, 2022 10:21

ematipico temporarily deployed to aws August 3, 2022 10:21 Inactive

leops approved these changes Aug 3, 2022

View reviewed changes

ematipico changed the title ~~feat(rome_service): new --apply-suggested argument to rome check~~ feat(rome_service): --apply-suggested argument to rome check Aug 4, 2022

ematipico merged commit 6d725db into main Aug 4, 2022

ematipico deleted the fix/check-apply-command branch August 4, 2022 07:34

IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022

feat(rome_service): --apply-suggested argument to rome check (rom…

1fe8ba5

…e#2952)

ematipico added A-CLI Area: CLI and removed A-CLI Area: CLI labels Aug 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(rome_service): `--apply-suggested` argument to `rome check` #2952

feat(rome_service): `--apply-suggested` argument to `rome check` #2952

Uh oh!

ematipico commented Jul 28, 2022 •

edited

Loading

Uh oh!

github-actions bot commented Jul 28, 2022 •

edited

Loading

Uh oh!

cloudflare-workers-and-pages bot commented Jul 28, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leops Jul 29, 2022

Uh oh!

ematipico Aug 2, 2022

Uh oh!

leops Aug 2, 2022

Uh oh!

MichaReiser commented Jul 29, 2022

Uh oh!

MichaReiser left a comment

Uh oh!

Uh oh!

ematipico commented Aug 2, 2022 •

edited

Loading

Uh oh!

ematipico commented Aug 2, 2022 •

edited

Loading

Uh oh!

leops commented Aug 2, 2022

Uh oh!

Uh oh!

feat(rome_service): --apply-suggested argument to rome check #2952

feat(rome_service): --apply-suggested argument to rome check #2952

Uh oh!

Conversation

ematipico commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

--apply-suggested argument

Settings passed to format requests

Messaging of the rome check --apply command

Test Plan

Uh oh!

github-actions bot commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudflare-workers-and-pages bot commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leops Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

ematipico Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

leops Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented Jul 29, 2022

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ematipico commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ematipico commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leops commented Aug 2, 2022

Uh oh!

Uh oh!

feat(rome_service): `--apply-suggested` argument to `rome check` #2952

feat(rome_service): `--apply-suggested` argument to `rome check` #2952

ematipico commented Jul 28, 2022 •

edited

Loading

`--apply-suggested` argument

Messaging of the `rome check --apply` command

github-actions bot commented Jul 28, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 28, 2022 •

edited

Loading

ematipico commented Aug 2, 2022 •

edited

Loading

ematipico commented Aug 2, 2022 •

edited

Loading