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(grainfmt): Allow directory input & output #1274

Merged
merged 6 commits into from
May 29, 2022

Conversation

phated
Copy link
Member

@phated phated commented May 28, 2022

Closes #973

This adds directory input & output support for grain format. The implementation is very close to the graindoc implementation and they can probably be consolidated in the future.

The big breaking changes to support this are:

  1. We no longer have an --in-place flag. Instead, you need to specify the same file as the "output", e.g. grain format test.gr -o test.gr
  2. We no longer support input on stdin. The stdin support was primarily to support the old way our vscode plugin operated, so this shouldn't be an issue. (Side note: we should theoretically be able to support stdin, but ppx_deriving_cmdliner crashes when we try to make a positional argument optional 🤷 Positional arguments with option type crash cmdliner hammerlab/ppx_deriving_cmdliner#51 )

I ran the directory formatting against the stdlib and the only unformatted file was Regex. So I suggest y'all review that file to look for complaints/bugs with the formatter (and we'll probably want to file those separately).

@phated phated requested a review from a team May 28, 2022 20:49
Copy link
Member

@marcusroberts marcusroberts left a comment

Choose a reason for hiding this comment

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

Looks great! The new LSP formatter doesn’t need stdin as you say so happy to see that go for directory support instead.

regex format looks good too

@peblair
Copy link
Member

peblair commented May 29, 2022

breaking changes

@phated Should this be feat(grainfmt)!:?

Copy link
Member

@peblair peblair left a comment

Choose a reason for hiding this comment

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

Looks good (though see earlier comment), but on the squash can you make sure there is some mention of reformatting Regex, since this will touch the git blame for most of that file?

@phated
Copy link
Member Author

phated commented May 29, 2022

Should this be feat(grainfmt)!:?

The specific breaking changes are marked as breaking, so they will show up. All will be included in the squash.

can you make sure there is some mention of reformatting Regex, since this will touch the git blame for most of that file?

Will do!

@phated phated merged commit d3e7a33 into main May 29, 2022
@phated phated deleted the phated/grainformat-directory-support branch May 29, 2022 18:25
@github-actions github-actions bot mentioned this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grainfmt: Directory support
3 participants