-
Notifications
You must be signed in to change notification settings - Fork 7
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
Introduce unified formatter interface; Use for gofmt
and prettier
#360
Conversation
gofmt
and prettier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice bash! Looks clean. Having tests for run-gofmt and run-prettier would be nice!
|
There may be a chicken/egg probably regarding which change to roll out first, the frontend change or the nixmodules changes. |
I made this backwards compat w/ the current frontend (tested go and prettier w/ frontend). So I think we can just:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See small fix to go runtimeInputs.
Co-authored-by: Toby Ho <[email protected]>
Test for run-gofmt
…m/replit/nixmodules into bm/format-using-unified-interface
@@ -0,0 +1,165 @@ | |||
{ pkgs }: | |||
|
|||
pkgs.writeShellApplication { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for using shellcheck.
#!/bin/bash | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#!/bin/bash |
writeShellApplication will add this.
# Execute the command | ||
gofmt "''${gofmt_args[@]}" | ||
''; | ||
checkPhase = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nix tests 😍
Why
As I was starting to write the pid2 formatter, I was realizing that I was going to have to duplicate some of the same hacks for prettier that I added to the frontend for pid1 (which formats via exec).
We have a unified interface on the frontend to initiate formatting, but individual formatters do not have a unified set of arguments. So in order to support some special arguments for prettier (like range formatting, and tab and spaces settings) I had to explicitly pass in arguments https://github.com/replit/repl-it-web/blob/b2f9ae504907d097e137c152eb295dd7cdaba78c/client/codemirror/formatter/useFormattingProviders.ts#L30
I was going to have to do the same thing in the pid2 formatter, unless I came up with a better solution. In the past I wrote a doc proposing writing all formatters as language servers to solve this problem. However, that was likely going to be a lot of work. We can end up in the same place, with a unified interface for the frontend to call, by just writing some bash scripts.
Here's the way it works:
-f | --file
: filepath - required-a
: if the formatter should write the changes directly, rather than returning the formatted string via stdout--range-start
: an offset, for supporting range formatting--range-end
: see aboveDescribe what prompted you to make this change, link relevant resources
What changed
I've implemented the above for both formatters we have in nixmodules today, gofmt and prettier. Both should be implemented in a backwards-compatible way to work with the current frontend setup. After this rolls out theres some frontend cleanup that can be done, that will then allow me to remove some of the TODOs in this PR.
Describe what changed to a level of detail that someone with no context with your PR could be able to review it
Test plan
I built both nodejs-20 and go locally using
I then inspected the active modules using
and manually ran the built formatter scripts against some typescript and go files to confirm they worked.
Tested e2e with frontend here https://replit.com/@replit-dev/brady-nixmodules#index.ts
Add
result.json
to modules array in .replitAble to format a typescript file with prettier
Able to format a golang file with gofmt. The nice thing is this actually fixes a bug with gofmt that currently formats all files, instead of just your active one.
Describe what you did to test this change to a level of detail that allows your reviewer to test it
Rollout
Describe any procedures or requirements needed to roll this out safely (or check the box below)