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

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.

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 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 3 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `grit format` command #595

implement `grit format` command #595

Arian8j2 commented Jan 1, 2025

algora-pbc bot commented Jan 1, 2025

morgante commented Jan 3, 2025

Arian8j2 Jan 4, 2025 •

edited

Loading

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025 •

edited

Loading

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025 •

edited

Loading

Arian8j2 Jan 4, 2025

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025

Arian8j2 Jan 4, 2025 •

edited

Loading

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025

morgante Jan 7, 2025

morgante Jan 7, 2025

morgante Jan 7, 2025

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025 •

edited

Loading

morgante Jan 7, 2025

morgante Jan 7, 2025

Arian8j2 Jan 8, 2025

implement grit format command #595

Are you sure you want to change the base?

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

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

implement `grit format` command #595

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 7, 2025 •

edited

Loading