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

feat: implement grit format command #595

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
561ce1d
feat: implement proof of concept of format command
Arian8j2 Jan 1, 2025
03d22b2
refactor: move format for loop body to different function
Arian8j2 Jan 2, 2025
7b49f63
feat: add tests for markdown and grit files (yaml currently has issues)
Arian8j2 Jan 2, 2025
fc431ec
feat: add yaml support
Arian8j2 Jan 2, 2025
7f767a4
feat: make file path bold in print
Arian8j2 Jan 2, 2025
3b0de9e
feat: change format command help message
Arian8j2 Jan 3, 2025
5761689
refactor: better explanation of grit_code_with_yaml_indent
Arian8j2 Jan 3, 2025
203de80
refactor: rewrite file using hunks instead of find and replace
Arian8j2 Jan 3, 2025
18e038f
feat: use better and consistent way of formating yaml files
Arian8j2 Jan 3, 2025
ea82025
chore: change some comments and variable names
Arian8j2 Jan 3, 2025
5ae0ebd
fix: resolves not being sorted currectly
Arian8j2 Jan 3, 2025
b744fdd
refactor: resolv todo
Arian8j2 Jan 4, 2025
f9b05e9
refactor: rename some functions and variables
Arian8j2 Jan 4, 2025
5e697ad
chore: fix clippy warnings
Arian8j2 Jan 4, 2025
1807c3c
refactor: better format .grit files
Arian8j2 Jan 4, 2025
570ff55
revert: grit range pointing at pattern definition
Arian8j2 Jan 4, 2025
828a2b5
feat: unwrap instead of returning error when it's safe
Arian8j2 Jan 4, 2025
3dcc719
chore: format yaml.rs
Arian8j2 Jan 4, 2025
9a13522
chore: remove todo question that i need to ask in review
Arian8j2 Jan 4, 2025
1c11a9f
refactor: return some errors instead of unwrapping
Arian8j2 Jan 7, 2025
e97fba8
chore: fix typos
Arian8j2 Jan 7, 2025
54076c7
feat: format files in parallel
Arian8j2 Jan 7, 2025
b049607
refactor: remove useless reference
Arian8j2 Jan 7, 2025
a977855
test: remove format_patterns test that snapshots the stdout
Arian8j2 Jan 7, 2025
f593bdd
test: add test for not parsable patterns
Arian8j2 Jan 7, 2025
9e7c24b
feat: use grit pattern to find and replace grit code in yaml
Arian8j2 Jan 10, 2025
0af5bf0
chore: fix comment
Arian8j2 Jan 11, 2025
409dcb3
refactor: format_yaml_file take patterns by reference
Arian8j2 Jan 11, 2025
7bb2622
chore: fix clippy warnings
Arian8j2 Jan 11, 2025
692673d
refactor: remove usesless clones
Arian8j2 Jan 11, 2025
ac8a0a8
refactor: apply hunk changes in reverse to make our life easier
Arian8j2 Jan 11, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
800 changes: 655 additions & 145 deletions Cargo.lock

Large diffs are not rendered by default.

5 changes: 4 additions & 1 deletion crates/cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

@Arian8j2 Arian8j2 Jan 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@Arian8j2 Arian8j2 Jan 7, 2025

Choose a reason for hiding this comment

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

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"] }

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok this is fine then.

Copy link
Author

@Arian8j2 Arian8j2 Jan 7, 2025

Choose a reason for hiding this comment

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

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

serde = { version = "1.0.164", features = ["derive"] }
serde = { version = "1.0.217", features = ["derive"] }
serde_json = { version = "1.0.96" }
serde_yaml = { version = "0.9.25" }
uuid = { version = "1.1", features = ["v4", "serde"] }
tokio = { version = "1", features = ["full"] }
chrono = { version = "0.4.26", features = ["serde"] }
Expand Down Expand Up @@ -88,6 +89,8 @@ tracing-subscriber = { version = "0.3", default-features = false, optional = tru
tracing-log = { version = "0.2.0", optional = true }

fs-err = { version = "2.11.0" }
biome_grit_parser = { git = "https://github.com/biomejs/biome" }
biome_grit_formatter = { git = "https://github.com/biomejs/biome" }

[target.'cfg(not(windows))'.dependencies]
openssl = { version = "0.10", features = ["vendored"] }
Expand Down
176 changes: 176 additions & 0 deletions crates/cli/src/commands/format.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
use crate::{
resolver::{resolve_from_cwd, Source},
ux::format_diff,
};
use anyhow::{anyhow, ensure, Context, Result};
use biome_grit_formatter::context::GritFormatOptions;
use clap::Args;
use colored::Colorize;
use marzano_gritmodule::{config::ResolvedGritDefinition, parser::PatternFileExt};
use serde::Serialize;

#[derive(Args, Debug, Serialize, Clone)]
pub struct FormatArgs {
/// Write formats to file instead of just showing them
#[clap(long)]
pub write: bool,
}

pub async fn run_format(arg: &FormatArgs) -> Result<()> {
let (mut resolved, _) = resolve_from_cwd(&Source::Local).await?;
// sort to have consistent output for tests
resolved.sort();

let file_path_to_resolved = group_resolved_patterns_by_group(resolved);
for (file_path, resovled_patterns) in file_path_to_resolved {
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

done

if let Err(error) =
format_file_resovled_patterns(file_path.clone(), resovled_patterns, arg.clone()).await
{
eprintln!("couldn't format '{}': {error:?}", file_path)
}
}
Ok(())
}

fn group_resolved_patterns_by_group(
resolved: Vec<ResolvedGritDefinition>,
) -> Vec<(String, Vec<ResolvedGritDefinition>)> {
resolved.into_iter().fold(Vec::new(), |mut acc, resolved| {
let file_path = &resolved.config.path;
if let Some((_, resolved_patterns)) = acc
.iter_mut()
.find(|(resolv_file_path, _)| resolv_file_path == file_path)
{
resolved_patterns.push(resolved);
} else {
acc.push((file_path.clone(), vec![resolved]));
}
acc
})
}

async fn format_file_resovled_patterns(
Arian8j2 marked this conversation as resolved.
Show resolved Hide resolved
file_path: String,
patterns: Vec<ResolvedGritDefinition>,
arg: FormatArgs,
) -> Result<()> {
// patterns has atleast one resolve so unwrap is safe
let first_pattern = patterns.first().unwrap();
Arian8j2 marked this conversation as resolved.
Show resolved Hide resolved
// currently all patterns has raw data so unwrap is safe
let first_pattern_raw_data = first_pattern.config.raw.as_ref().unwrap();
let old_file_content = &first_pattern_raw_data.content;

let new_file_content = match first_pattern_raw_data.format {
PatternFileExt::Yaml => format_yaml_file(old_file_content)?,
PatternFileExt::Grit => format_grit_code(old_file_content)?,
PatternFileExt::Md => {
let hunks = patterns
.iter()
.map(format_pattern_as_hunk_changes)
.collect::<Result<Vec<HunkChange>>>()?;
apply_hunk_changes(old_file_content, hunks)
}
};

if &new_file_content == old_file_content {
return Ok(());
}

if arg.write {
tokio::fs::write(file_path, new_file_content)
.await
.with_context(|| "could not write to file")?;
} else {
println!(
"{}:\n{}",
file_path.bold(),
format_diff(old_file_content, &new_file_content)
);
}

Ok(())
}

fn format_yaml_file(file_content: &str) -> Result<String> {
Copy link
Author

@Arian8j2 Arian8j2 Jan 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Arian8j2 Arian8j2 Jan 10, 2025

Choose a reason for hiding this comment

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

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

// deserializing manually and not using `SerializedGritConfig` because
// i don't want to remove any fields that `SerializedGritConfig` don't have such as 'version'
let mut config: serde_yaml::Value =
serde_yaml::from_str(file_content).with_context(|| "couldn't parse yaml file")?;
let patterns = config
.get_mut("patterns")
.ok_or_else(|| anyhow!("couldn't find patterns in yaml file"))?
.as_sequence_mut()
.ok_or_else(|| anyhow!("patterns in yaml file are not sequence"))?;
for pattern in patterns {
let Some(body) = pattern.get_mut("body") else {
continue;
};
if let serde_yaml::Value::String(body_str) = body {
*body_str = format_grit_code(body_str)?;
// extra new line at end of grit body looks more readable
body_str.push('\n');
}
}
Ok(serde_yaml::to_string(&config)?)
}

fn format_pattern_as_hunk_changes(pattern: &ResolvedGritDefinition) -> Result<HunkChange> {
let formatted_grit_code = format_grit_code(&pattern.body)?;
let body_range = pattern
.config
.range
.ok_or_else(|| anyhow!("pattern doesn't have config range"))?;
Ok(HunkChange {
starting_byte: body_range.start_byte as usize,
ending_byte: body_range.end_byte as usize,
new_content: formatted_grit_code,
})
}

/// format grit code using `biome`
fn format_grit_code(source: &str) -> Result<String> {
let parsed = biome_grit_parser::parse_grit(source);
ensure!(
parsed.diagnostics().is_empty(),
"biome couldn't parse: {}",
parsed
.diagnostics()
.iter()
.map(|diag| diag.message.to_string())
.collect::<Vec<_>>()
.join("\n")
);

let options = GritFormatOptions::default();
let doc = biome_grit_formatter::format_node(options, &parsed.syntax())
.with_context(|| "biome couldn't format")?;
Ok(doc.print()?.into_code())
}

/// Represent a hunk of text that needs to be changed
#[derive(Debug)]
struct HunkChange {
starting_byte: usize,
ending_byte: usize,
new_content: String,
}

/// returns a new string that applies hunk changes
fn apply_hunk_changes(input: &str, mut hunks: Vec<HunkChange>) -> String {
if hunks.is_empty() {
return input.to_string();
}
hunks.sort_by_key(|hunk| hunk.starting_byte);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Nice idea, Done

let mut buffer = String::new();
let mut last_ending_byte = 0;
for (index, hunk) in hunks.iter().enumerate() {
buffer.push_str(&input[last_ending_byte..hunk.starting_byte]);
buffer.push_str(&hunk.new_content);
last_ending_byte = hunk.ending_byte;

if index == hunks.len() - 1 {
buffer.push_str(&input[last_ending_byte..]);
}
}
buffer
}
Arian8j2 marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 6 additions & 0 deletions crates/cli/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub(crate) mod patterns_list;
pub(crate) mod patterns_test;
pub(crate) mod plumbing;
pub(crate) mod version;
pub(crate) mod format;

#[cfg(feature = "workflows_v2")]
pub(crate) mod apply_migration;
Expand Down Expand Up @@ -84,6 +85,7 @@ use indicatif_log_bridge::LogWrapper;
use init::InitArgs;
use install::InstallArgs;
use list::ListArgs;
use format::{run_format, FormatArgs};
use log::LevelFilter;
use lsp::LspArgs;
use marzano_messenger::emit::ApplyDetails;
Expand Down Expand Up @@ -164,6 +166,8 @@ pub enum Commands {
Plumbing(PlumbingArgs),
/// Display version information about the CLI and agents
Version(VersionArgs),
/// Format grit files under current directory
Format(FormatArgs),
/// Generate documentation for the Grit CLI (internal use only)
#[cfg(feature = "docgen")]
#[clap(hide = true)]
Expand Down Expand Up @@ -204,6 +208,7 @@ impl fmt::Display for Commands {
},
Commands::Plumbing(_) => write!(f, "plumbing"),
Commands::Version(_) => write!(f, "version"),
Commands::Format(_) => write!(f, "format"),
#[cfg(feature = "docgen")]
Commands::Docgen(_) => write!(f, "docgen"),
#[cfg(feature = "server")]
Expand Down Expand Up @@ -439,6 +444,7 @@ async fn run_command(_use_tracing: bool) -> Result<()> {
run_plumbing(arg, multi, &mut apply_details, app.format_flags).await
}
Commands::Version(arg) => run_version(arg).await,
Commands::Format(arg) => run_format(&arg).await,
#[cfg(feature = "docgen")]
Commands::Docgen(arg) => run_docgen(arg).await,
#[cfg(feature = "server")]
Expand Down
2 changes: 1 addition & 1 deletion crates/cli/src/ux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl fmt::Display for DiffString {
}
}

fn format_diff(expected: &str, actual: &str) -> DiffString {
pub fn format_diff(expected: &str, actual: &str) -> DiffString {
let mut output = String::new();
let diff = TextDiff::from_lines(expected, actual);

Expand Down
12 changes: 12 additions & 0 deletions crates/cli_bin/fixtures/unformatted_patterns/.grit/grit.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
private: true
tags: [private]
---
```grit
language js
`sanitizeFilePath` as $s where {
move_import(`sanitizeFilePath`, `'@getgrit/universal'`)
}
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
---
title: Aspect ratio
---

```grit
language css

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

## Matches the right selector and declaration block

```css
a {
width: calc(100% - 80px);
aspect-ratio: 1/2;
font-size: calc(10px + (56 - 10) * ((100vw - 320px) / (1920 - 320)));
}

#some-id {
some-property: 5px;
}

a.b ~ c.d {
}
.e.f + .g.h {
}

@font-face {
font-family: 'Open Sans';
src: url('/a') format('woff2'), url('/b/c') format('woff');
}
```

```css
a {
width: calc(100% - 80px);
aspect-ratio: 1/2;
font-size: calc(10px + (56 - 10) * ((100vw - 320px) / (1920 - 320)));
}

#some-id {
some-property: 5px;
}

a.b ~ c.d {
}
.e.f + .g.h {
}

@font-face {
font-family: 'Open Sans';
src: url('/a') format('woff2'), url('/b/c') format('woff');
}
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
language json

pattern upgrade_dependency($target_dep, $target_version, $dependency_key) {
or {
`$key: $value` where {
$key <: `"$target_dep"`,
$value => `"$target_version"`
},
pair($key, $value) where {
$key <: `"$dependency_key"`,
$value <: object($properties) where {
$properties <: not contains pair(key=$dep_key) where {
$dep_key <: contains `$target_dep`
},
$properties => `"$target_dep": "$target_version",\n$properties`
}
}
}
}

pattern console_method_to_info($method) {
`console.$method($message)` => `console.info($message)`
}
Loading
Loading