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

Introduce unified formatter interface; Use for gofmt and prettier #360

Merged
merged 10 commits into from
Jun 25, 2024

Conversation

bradymadden97
Copy link
Contributor

@bradymadden97 bradymadden97 commented Jun 25, 2024

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:

  1. Define a common "formatter" interface and set of options that the frontend can send. For this I used the following:
  • -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 above
  • the various LSP formatting options as defined in the spec
  1. The frontend will pass its desired options according to the common spec
  2. Each formatter, within nixmodules, will define its own script that will receive the parsed common options, and translate them to binary-specific options before calling its formatter binary. If it doesn't support one of the common options, it can just ignore it

Describe 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

nix build .#custom-bundle

I then inspected the active modules using

nix eval .#activeModules --json | jq

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

nix build .#'"nodejs-20"'
ln -s result result.json

Add result.json to modules array in .replit
Able to format a typescript file with prettier

nix build .#'"go-1.21"'
ln -s result result.json

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)

  • This is fully backward and forward compatible

@bradymadden97 bradymadden97 requested a review from a team as a code owner June 25, 2024 18:03
@bradymadden97 bradymadden97 requested review from ryantm and removed request for a team June 25, 2024 18:03
@bradymadden97 bradymadden97 changed the title make unified formatter interface, and use for gofmt and prettier Introduce unified formatter interface; Use for gofmt and prettier Jun 25, 2024
Copy link
Collaborator

@airportyh airportyh left a 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!

@airportyh
Copy link
Collaborator

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.
love this!

@airportyh
Copy link
Collaborator

There may be a chicken/egg probably regarding which change to roll out first, the frontend change or the nixmodules changes.

@bradymadden97
Copy link
Contributor Author

I made this backwards compat w/ the current frontend (tested go and prettier w/ frontend).

So I think we can just:

  1. roll this out w/ no frontend changes
  2. then I will update the frontend to remove the prettier specific stuff and use some of the unified args
  3. then i will come back here and remove some of the TODOs that were added for backwards-compat reasons

Copy link
Collaborator

@airportyh airportyh left a 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.

@bradymadden97 bradymadden97 merged commit 1835908 into main Jun 25, 2024
1 check passed
@bradymadden97 bradymadden97 deleted the bm/format-using-unified-interface branch June 25, 2024 21:40
@@ -0,0 +1,165 @@
{ pkgs }:

pkgs.writeShellApplication {
Copy link
Collaborator

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.

Comment on lines +7 to +8
#!/bin/bash

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#!/bin/bash

writeShellApplication will add this.

# Execute the command
gofmt "''${gofmt_args[@]}"
'';
checkPhase = ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

nix tests 😍

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.

3 participants