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

grit format command #574

Open
morgante opened this issue Nov 13, 2024 · 21 comments · May be fixed by #595
Open

grit format command #574

morgante opened this issue Nov 13, 2024 · 21 comments · May be fixed by #595

Comments

@morgante
Copy link
Contributor

morgante commented Nov 13, 2024

Now that biome has added support for Grit as a formattable language we should add a grit format command to the Grit CLI. We can (hopefully) depend on the Biome crate for this.

grit format should:

  • Look for all grit patterns in the current directory (using our existing mechanisms, like grit list), including grit.yaml, *.grit files, and markdown patterns
  • If a --write flag is provided, write the re-formatted patterns back. Otherwise just display the diff / changes

Use https://github.com/getgrit/stdlib for a repo to test on, but add at least one CLI test case in https://github.com/getgrit/gritql/tree/main/crates/cli_bin/tests

@morgante
Copy link
Contributor Author

/bounty $200

Copy link

algora-pbc bot commented Nov 13, 2024

💎 $200 bounty • Grit

Steps to solve:

  1. Start working: Comment /attempt #574 with your implementation plan
  2. Submit work: Create a pull request including /claim #574 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to getgrit/gritql!

Add a bountyShare on socials

Attempt Started (GMT+0) Solution
🟢 @harshtech123 Nov 13, 2024, 8:43:59 AM WIP
🟢 @vishwamartur Nov 18, 2024, 7:30:23 AM WIP
🟢 @Arian8j2 Jan 1, 2025, 4:21:22 PM #595

@tareknaser
Copy link

Hello
I’d like to take this on. Could I get assigned?

@morgante
Copy link
Contributor Author

I do not assign issues to first-time contributors. You're welcome to attempt it.

@harshtech123
Copy link

harshtech123 commented Nov 13, 2024

/attempt #574

@tareknaser
Copy link

I ran into an issue while implementing this. Is there a way to list only the grit patterns in the current directory? It looks like grit list always includes the standard library workflows too.
After tracing the code, I noticed that the _stdlib_workflows boolean parameter in the list_applyables function (located in crates/cli/src/commands/list.rs) is being completely ignored.

@morgante
Copy link
Contributor Author

Look at how grit patterns test works.

@tareknaser
Copy link

Thanks. I updated the code to follow a similar pattern to what's in grit patterns test, and it's working as expected.

I've made progress on this but ran into a blocker, and I’d appreciate your input.

The formatter is working locally by depending on the biome crates, and I verified it by running it on examples here, which worked as expected. I’ve also set up the diff and apply functionality if write is set to true, so that part is sorted.

However, the issue is that the formatter doesn’t actually support the files we want to format. Specifically, it doesn’t support markdown patterns and .grit files with the format seen in the stdlib (e.g., common.grit).

For instance, running the biome formatter on .grit/patterns/go/common.grit fails.

Am I missing something here?

@morgante
Copy link
Contributor Author

Specifically, it doesn’t support markdown patterns

This is expected. You need to extract the gritql from the markdown file and format the extracted gritql then write it back in. We already have logic (in gritmodules) that handles extracting patterns from markdown.

For instance, running the biome formatter on .grit/patterns/go/common.grit fails.

What fails? We should patch that upstream.

@tareknaser
Copy link

This is expected. You need to extract the gritql from the markdown file and format the extracted gritql then write it back in.

This seems like a special case we could address in a follow-up PR. I have the main functionality set up locally, including the grit format command with a write flag, and it scans the current directory as expected.

Do you think I should open a PR for this?

What fails?

It’s a bit inconsistent at the moment—it works for some .grit files, but not all. For example, .grit/patterns/go/go_imports.grit fails for me. Here’s the error I’m getting:

---- format_write stdout ----
thread 'main' panicked at /Users/tareknasser/.cargo/git/checkouts/biome-d49ce8898b420a50/8a90b89/crates/biome_parser/src/diagnostic.rs:350:14:
Expected token to be a punctuation or keyword.

@morgante
Copy link
Contributor Author

This seems like a special case we could address in a follow-up PR. I have the main functionality set up locally, including the grit format command with a write flag, and it scans the current directory as expected.

It's not a special case. I called it out as core functionality that is intrinsic to this ticket.

Do you think I should open a PR for this?

You can, but the bounty will not be awarded until the issue is completed. You must handle .md and .yaml files, as stated in the spec.

It’s a bit inconsistent at the moment—it works for some .grit files, but not all.

That's a biome issue, so you can ignore it. It's out of scope for this issue.

@vishwamartur
Copy link

vishwamartur commented Nov 18, 2024

/attempt #574

vishwamartur added a commit to vishwamartur/gritql that referenced this issue Nov 18, 2024
Related to getgrit#574

Add `grit format` command to the Grit CLI.

* Implement the `grit format` command in `crates/cli/src/commands/format.rs` using the Biome crate for formatting.
* Add logic to look for all grit patterns in the current directory, including `.yaml`, `.grit` files, and markdown patterns.
* Implement the `--write` flag to write the re-formatted patterns back if provided.
* Extract gritql from markdown files and format the extracted gritql.
* Update `crates/cli/src/commands/mod.rs` to include the `format` module and the `Format` variant in the `Commands` enum.
* Add a CLI test case for the `grit format` command in `crates/cli_bin/tests/format.rs`.
@fvckDesa
Copy link

@morgante is this issue open for contribution?

@morgante
Copy link
Contributor Author

morgante commented Jan 1, 2025

@morgante is this issue open for contribution?

Yes.

@Arian8j2
Copy link

Arian8j2 commented Jan 1, 2025

Hello, I'm a bit confused. Should the format command utilize the biome crate for formatting, while also implementing a formatting mechanism for file types that biome does not support, such as markdown?

@morgante
Copy link
Contributor Author

morgante commented Jan 1, 2025

Already answered. You need to extract the grit and format that.

@Arian8j2
Copy link

Arian8j2 commented Jan 1, 2025

Doesn't biome already do that? Does biome just format one file?

@morgante
Copy link
Contributor Author

morgante commented Jan 1, 2025

  1. Biome does not support Markdown/yaml patterns.
  2. I want the grit CLI to ship an integrated toolchain.

At this point I have low confidence in bounty hunters doing this task correctly.

@Arian8j2
Copy link

Arian8j2 commented Jan 1, 2025

Oh, I think i got it, The grit format command should format grit pattern files such as .grit, .yaml and .md files, i thought it should format files under current directory using grit patterns, Sorry for useless questions

@Arian8j2
Copy link

Arian8j2 commented Jan 1, 2025

/attempt #574

@Arian8j2 Arian8j2 linked a pull request Jan 1, 2025 that will close this issue
Copy link

algora-pbc bot commented Jan 1, 2025

💡 @Arian8j2 submitted a pull request that claims the bounty. You can visit your bounty board to reward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants