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(format/grit): add logic for formatting with separators and other predicates #4327

Merged
merged 3 commits into from
Oct 19, 2024

Conversation

branberry
Copy link
Contributor

@branberry branberry commented Oct 17, 2024

Summary

Add the ability to handle multi-line patterns that are separated with commas.

Added simple formatting rules for:

  • accumulator predicate
  • match predicate
  • rewrite predicate

Test Plan

Snapshot tests demonstrate correct formatting

@github-actions github-actions bot added A-Formatter Area: formatter L-Grit Language: GritQL labels Oct 17, 2024
Copy link

codspeed-hq bot commented Oct 17, 2024

CodSpeed Performance Report

Merging #4327 will not alter performance

Comparing branberry:feat/separator (ab6c8d7) with main (8931d43)

Summary

✅ 101 untouched benchmarks

@ematipico
Copy link
Member

Snapshot tests demonstrate correct formatting

Sorry @branberry, but I don't see any changes in the snapshots. The input and the output are the same. Usually, it's very useful to have your input unformatted (add some spaces or newlines around the implemented nodes), and if the output is different, and it's formatted the way you want. I suggest to add an unformatted case to your snapshot and also keep the formatted one.

https://github.com/branberry/biome/blob/8e243aee5da523897897a69c11f27f59a2381375/crates/biome_grit_formatter/tests/specs/grit/patterns/create_new_files.grit.snap#L5-L38

@branberry
Copy link
Contributor Author

branberry commented Oct 18, 2024

Oh, shoot! No need to apologize @ematipico. My apologies, I missed it. I thought I saw the correct formatting changes locally. Let me adjust this so that we have the desired formatting in the snapshot results.

Thank you!

EDIT:

On second thought, I think I do have the formatting, but it's quite subtle. From the snapshots:

Input

`function $functionName($_) {$_}` as $f where{
  $functionName<:r"test.*",
  $f=>.,
  $new_file_name=`$functionName.test.js`,
  $new_files+=file(name = $new_file_name, body = $f)
}

Output

`function $functionName($_) {$_}` as $f where {
	$functionName <: r"test.*",
	$f => .,
	$new_file_name = `$functionName.test.js`,
	$new_files += file(name = $new_file_name, body = $f)
}

I've just added spaces more or less around assignments, which may or may not be what is desired in terms of formatting here.

Either way, I think this points to an area where I can improve, which is to make the tests I write more clear. Thanks again!!

@ematipico
Copy link
Member

ematipico commented Oct 18, 2024

Don't worry, it's completely fine. This was more an advice from a person who worked from the very beginning on the formatter. While things change, sometimes it's difficult to understand the wanted/expeted formatting.

Having a "completely wrong" version of the input is a good way to visually see what changes

Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

🚀

@ematipico ematipico merged commit e9d3b6e into biomejs:main Oct 19, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter L-Grit Language: GritQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants