feat: implement `grit format` command #595

Arian8j2 · 2025-01-01T16:24:56Z

trying to resolve #574
this is just a proof of concept right now and needs more work and time to get complete
/claim #574

algora-pbc · 2025-01-01T16:25:05Z

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe.

morgante · 2025-01-03T03:01:17Z

Thanks @Arian8j2 for working on this, just wanted to chime in that it's definitely on the right track for what I had in mind. Once you're ready, feel free to request a review.

Arian8j2 · 2025-01-04T07:26:51Z

crates/cli/Cargo.toml

@@ -20,8 +20,9 @@ anyhow = { version = "1.0.70" }
 clap = { version = "4.1.13", features = ["derive"] }
 indicatif = { version = "0.17.5" }
 # Do *NOT* upgrade beyond 1.0.171 until https://github.com/serde-rs/serde/issues/2538 is fixed


@morgante Is this still important because biome depends on serde version 1.0.217?

Let's avoid changing this in the same PR if we can.

after looking deeper into this, i found out that the main branch doesn't even use serde version under 1.0.171 😄, cargo lock of main branch shows:

[[package]] name = "serde" version = "1.0.208" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cff085d2cb684faa248efb494c39b68e522822ac0de72ccf08109abde717cfb2" dependencies = [ "serde_derive", ]

even in the very first commit 79a5365, cargo lock was:

[[package]] name = "serde" version = "1.0.197" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3fb1c873e1b9b056a4dc4c0c198b24c3ffa059243875552b2bd0933b1aee4ce2" dependencies = [ "serde_derive", ]

the current cargo toml is:

serde = { version = "1.0.164", features = ["derive"] }

this allows versions >= 1.0.164, < 2.0.0 according to the cargo book, for exact 1.0.164 version the toml should looked like:

serde = { version = "=1.0.164", features = ["derive"] }

or for under 1.0.171 it should looked like this:

serde = { version = "< 1.0.171", features = ["derive"] }

Ok this is fine then.

ok then do you want me to remove that comment to avoid confusion later?

Arian8j2 · 2025-01-04T07:28:53Z

crates/cli/src/commands/format.rs

+    resolved.sort();
+
+    let file_path_to_resolved = group_resolved_patterns_by_group(resolved);
+    for (file_path, resovled_patterns) in file_path_to_resolved {


This can be easily executed in parallel, however the tests that verify stdout will fail due to inconsistencies in the output

Instead of changing the input to make tests happy, another option is to sort the test output by line so it is stabilized.

Arian8j2 · 2025-01-04T07:29:32Z

crates/cli/src/commands/format.rs

+    Ok(())
+}
+
+fn format_yaml_file(file_content: &str) -> Result<String> {


@morgante Is it ok to also format the whole yaml file?

What's the reason?

because we need to parse the whole yaml and edit the body and then serialize it back to yaml format, in this way the yaml code also get's formatted. i couldn't find a way to just change body field, also because it's usually multi line string it's also hard to just find the bytes and replace it, because we need additional space and tabs to make it valid yaml code.

We should not not serialize/deserialize the whole body. Round-tripping is lossy. We'd also lose any comments in the yaml.

I understand finding and replacing just the bytes is harder but it's very much possible.

the problem is in finding the code, for example consider this yaml file:

version: 0.0.1 patterns: - name: aspect_ratio_yaml description: Yaml version of aspect_ratio.md body: | language css `a { $props }` where { $props <: contains `aspect-ratio: $x` } - file: ./others/test_move_import.md

i can easily get the grit code:

language css `a { $props }` where { $props <: contains `aspect-ratio: $x` }

and format it no problem, but then changing the body with new formatted code is difficult because how i find the old grit code in the file? each line is prefixed with indent spaces:

language css `a { $props }` where { $props <: contains `aspect-ratio: $x` }

i need to know how much space i need to put before the code (i used to hard code this in the past commits but then it introduces other problems), thats the hard part, and i couldn't think of a way that is not hacky and breakable

i could try to find the code like this:

for i in 1..10 { let indent = " ".repeat(i); let grit_code_with_indent = prefix_each_line(indent, grit_code) if let Some(pos) = yaml_file_content.position(grit_code_with_indent) { // ... break } }

but it's a bit hacky

We actually have a lot of utils in GritQL itself for handling indentation—tested here—so I think you should be able to leverage those.

nice hint, GritQL really shines in these issues but i don't know if it's overkill or not for this, btw i updated the code, now it uses a grit pattern to find and replace yaml bodies with formatted ones

crates/cli/src/commands/format.rs

morgante · 2025-01-07T06:53:19Z

crates/cli/Cargo.toml

@@ -20,8 +20,9 @@ anyhow = { version = "1.0.70" }
 clap = { version = "4.1.13", features = ["derive"] }
 indicatif = { version = "0.17.5" }
 # Do *NOT* upgrade beyond 1.0.171 until https://github.com/serde-rs/serde/issues/2538 is fixed


Let's avoid changing this in the same PR if we can.

morgante · 2025-01-07T06:54:31Z

crates/cli/src/commands/format.rs

+    resolved.sort();
+
+    let file_path_to_resolved = group_resolved_patterns_by_group(resolved);
+    for (file_path, resovled_patterns) in file_path_to_resolved {


Instead of changing the input to make tests happy, another option is to sort the test output by line so it is stabilized.

crates/cli/src/commands/format.rs

morgante · 2025-01-07T06:55:36Z

crates/cli/src/commands/format.rs

+    Ok(())
+}
+
+fn format_yaml_file(file_content: &str) -> Result<String> {


What's the reason?

crates/cli/src/commands/format.rs

morgante · 2025-01-07T06:57:41Z

crates/cli_bin/tests/format.rs

+        output.status.success(),
+        "Command didn't finish successfully"
+    );
+    assert_yaml_snapshot!(String::from_utf8(output.stdout)?);


Instead of snapshotting the test output, leading to these problems, snapshot the final state of the file (after formatting) + assert on test output containing known messages.

leading to these problems

what problems? the link just opens pull request file changes.
also, in format_patterns_with_rewrite i snapshot the file contents, you mean don't snapshot the stdout at all? and only test with the write flag on?

assert on test output containing known messages

in case of errors? for example putting a file that can't be parsed and check for couldn't format file: parse error in stderr?

in case of errors? for example putting a file that can't be parsed and check for couldn't format file: parse error in stderr?

Right.

what problems? the link just opens pull request file changes.

It links to the test snapshot.

Yes, you should avoid snapshot testing stdout. Focus snapshot testing the formatted files.

morgante · 2025-01-11T08:32:35Z

crates/cli/src/commands/format.rs

+    if hunks.is_empty() {
+        return input.to_string();
+    }
+    hunks.sort_by_key(|hunk| hunk.starting_byte);


Since you're sorting it, just reverse the order (apply last to first) and use replace_range.

Nice idea, Done

morgante · 2025-01-11T08:34:13Z

crates/cli/src/commands/format.rs

+/// bubble clause that finds a grit pattern with name "\<pattern_name\>" in yaml and
+/// replaces it's body to "\<new_body\>", `format_yaml_file` uses this pattern to replace
+/// pattern bodies with formatted ones
+const YAML_REPLACE_BODY_PATERN: &str = r#"


Composing a string then parsing it is going to be error prone / run into escaping issues.

Better to just build the pattern directly in Rust.

Composing a string then parsing it is going to be error prone / run into escaping issues.

Yeah, i was thinking about inserting variable to runtime and use it in the pattern, But couldn't find how to do it

Better to just build the pattern directly in Rust.

That's gonna be hard :), Is there any place that i can see examples of this?

That's gonna be hard :), Is there any place that i can see examples of this?

Unfortunately don't have many examples today, you would have to build it up via the different pattern structs. I'm probably ok merging this as-is.

Is escaping just double quote ok? if so yeah i think it can be merged.

Yeah double quotes should be sufficient.

morgante · 2025-01-12T05:35:40Z

@Arian8j2 Please make sure all tests / clippy pass.

Arian8j2 · 2025-01-12T06:08:41Z

@morgante Couldn't build biome with current cargo version of CI (1.76.0-nightly):

error[E0658]: the `#[expect]` attribute is an experimental feature
   --> .cargo/git/checkouts/biome-d49ce8898b420a50/8bec9fc/crates/biome_string_case/src/lib.rs:629:13
    |
629 |             #[expect(clippy::disallowed_methods)]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #54503 <https://github.com/rust-lang/rust/issues/54503> for more information
    = help: add `#![feature(lint_reasons)]` to the crate attributes to enable

error[E0658]: the `#[expect]` attribute is an experimental feature
   --> .cargo/git/checkouts/biome-d49ce8898b420a50/8bec9fc/crates/biome_string_case/src/lib.rs:645:13
    |
645 |             #[expect(clippy::disallowed_methods)]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #54503 <https://github.com/rust-lang/rust/issues/54503> for more information
    = help: add `#![feature(lint_reasons)]` to the crate attributes to enable

error[E0658]: the `#[expect]` attribute is an experimental feature
   --> .cargo/git/checkouts/biome-d49ce8898b420a50/8bec9fc/crates/biome_string_case/src/lib.rs:660:13
    |
660 |             #[expect(clippy::disallowed_methods)]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #54503 <https://github.com/rust-lang/rust/issues/54503> for more information
    = help: add `#![feature(lint_reasons)]` to the crate attributes to enable

Should we perhaps increase the CI rust version?

feat: implement proof of concept of format command

561ce1d

Arian8j2 requested a review from a team as a code owner January 1, 2025 16:24

algora-pbc bot mentioned this pull request Jan 1, 2025

grit format command #574

Open

algora-pbc bot added the 🙋 Bounty claim label Jan 1, 2025

Arian8j2 marked this pull request as draft January 1, 2025 16:25

Arian8j2 added 4 commits January 2, 2025 12:06

refactor: move format for loop body to different function

03d22b2

feat: add tests for markdown and grit files (yaml currently has issues)

7b49f63

feat: add yaml support

fc431ec

feat: make file path bold in print

7f767a4

Arian8j2 added 13 commits January 3, 2025 09:34

feat: change format command help message

3b0de9e

refactor: better explanation of grit_code_with_yaml_indent

5761689

refactor: rewrite file using hunks instead of find and replace

203de80

feat: use better and consistent way of formating yaml files

18e038f

chore: change some comments and variable names

ea82025

fix: resolves not being sorted currectly

5ae0ebd

refactor: resolv todo

b744fdd

refactor: rename some functions and variables

f9b05e9

chore: fix clippy warnings

5e697ad

refactor: better format .grit files

1807c3c

revert: grit range pointing at pattern definition

570ff55

feat: unwrap instead of returning error when it's safe

828a2b5

chore: format yaml.rs

3dcc719

Arian8j2 force-pushed the main branch from 772526b to 3dcc719 Compare January 4, 2025 07:21

chore: remove todo question that i need to ask in review

9a13522

Arian8j2 commented Jan 4, 2025

View reviewed changes

Arian8j2 marked this pull request as ready for review January 4, 2025 07:30

Arian8j2 commented Jan 4, 2025

View reviewed changes

crates/cli/src/commands/format.rs Outdated Show resolved Hide resolved

morgante reviewed Jan 7, 2025

View reviewed changes

Arian8j2 added 3 commits January 7, 2025 14:57

refactor: return some errors instead of unwrapping

1c11a9f

chore: fix typos

e97fba8

feat: format files in parallel

54076c7

Arian8j2 force-pushed the main branch from d04e971 to 54076c7 Compare January 7, 2025 12:21

Arian8j2 added 8 commits January 7, 2025 16:44

refactor: remove useless reference

b049607

test: remove format_patterns test that snapshots the stdout

a977855

test: add test for not parsable patterns

f593bdd

feat: use grit pattern to find and replace grit code in yaml

9e7c24b

chore: fix comment

0af5bf0

refactor: format_yaml_file take patterns by reference

409dcb3

chore: fix clippy warnings

7bb2622

refactor: remove usesless clones

692673d

morgante reviewed Jan 11, 2025

View reviewed changes

refactor: apply hunk changes in reverse to make our life easier

ac8a0a8

Arian8j2 changed the title ~~implement grit format command~~ feat: implement grit format command Jan 12, 2025

feat: implement grit format command #595

Are you sure you want to change the base?

feat: implement grit format command #595

Conversation

Arian8j2 commented Jan 1, 2025

algora-pbc bot commented Jan 1, 2025

morgante commented Jan 3, 2025

Arian8j2 Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arian8j2 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arian8j2 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arian8j2 Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arian8j2 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arian8j2 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgante Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Arian8j2 Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arian8j2 Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgante commented Jan 12, 2025

Arian8j2 commented Jan 12, 2025

feat: implement `grit format` command #595

feat: implement `grit format` command #595

Arian8j2 Jan 4, 2025 •

edited

Loading

Arian8j2 Jan 7, 2025 •

edited

Loading

Arian8j2 Jan 7, 2025 •

edited

Loading

Arian8j2 Jan 4, 2025 •

edited

Loading

Arian8j2 Jan 10, 2025 •

edited

Loading

Arian8j2 Jan 7, 2025 •

edited

Loading

morgante Jan 11, 2025 •

edited

Loading

Arian8j2 Jan 11, 2025 •

edited

Loading

Arian8j2 Jan 11, 2025 •

edited

Loading