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

cli: implement enough of jj fix to run a single tool on all files #3698

Merged
merged 4 commits into from
Jun 4, 2024

Conversation

hooper
Copy link
Contributor

@hooper hooper commented May 16, 2024

Our immediate motivation is to have something basic that will work with a code formatter multiplexing wrapper script thingy inside of Google. The goal is to get to feature parity with hg fix, minus a couple of bad parts.

The main difference to look out for here was mentioned in Martin's PR, which is that for efficiency we want to loop over file IDs instead of (file ID, commit ID) pairs. That comes with some complications to error reporting.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@hooper hooper force-pushed the push-yoyuqnpxtkql branch 2 times, most recently from d8a3327 to 59265e8 Compare May 16, 2024 19:38
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

just minor stuff and a question

cli/src/commands/fix.rs Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
@hooper hooper force-pushed the push-yoyuqnpxtkql branch from 59265e8 to ac2e9bd Compare May 16, 2024 21:31
@thoughtpolice
Copy link
Member

Just as a note, I think it would be nice if:

  1. jj fix took a fileset to express which paths to actually touch
  2. It would be nice if the user had some way of specifying the command per fileset in some way, rather than having a single blob which is expected to differentiate between all possible input files.

Maybe a map in the config, something like:

[fix]
"glob:**/BUILD" = "fix-bazel-files" # this is a command in $PATH
"glob:**/*.rs" = ["rustfmt", "..."] # include more arguments

Which would allow a more fine-grained approach.

Probably neither of these need to be handled on a first pass, though, and are relatively simple additions.

@martinvonz
Copy link
Member

  • jj fix took a fileset to express which paths to actually touch

Yes, that's coming in a later PR. I don't think @hooper has started working on it, but we're planning to add it, and I think it's trivial to add.

2. It would be nice if the user had some way of specifying the command per fileset in some way, rather than having a single blob which is expected to differentiate between all possible input files.

Also planned :) hg fix supports that. I think the plan is to do something similar.

@hooper hooper force-pushed the push-yoyuqnpxtkql branch from ac2e9bd to 3c119a4 Compare May 16, 2024 23:32
Cargo.lock Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
@martinvonz
Copy link
Member

btw, non-googlers: don't worry, i'm just helping review this but i'll leave approval for a non-googler

@hooper
Copy link
Contributor Author

hooper commented May 16, 2024

Also for non-Googlers, a lot of the context is in the documentation for the prior art in Mercurial: https://www.mercurial-scm.org/doc/hg.1.html#fix

cli/src/commands/fix.rs Outdated Show resolved Hide resolved
@PhilipMetzger
Copy link
Contributor

Maybe a map in the config, something like:

[fix]
"glob:**/BUILD" = "fix-bazel-files" # this is a command in $PATH
"glob:**/*.rs" = ["rustfmt", "..."] # include more arguments

Which would allow a more fine-grained approach.

In my opinion, fix is actual the perfect command for TOML tables, so you even could name the formatting/fixing action. So a eventual --verbose flag can print something like ran clang-format on <file>.cc, <file>.h.

This would fix some issues, which the Mercurial version has.

[fix]
# "patch" could mean accept the patch it generates and directly apply it.  
clang-format = { bin = "git-clangformat", files = "glob:**/*.h,**/*.cc", patch = true } 
rustfmt = { bin = "rustfmt", files = "glob:**/*.rs", args = "..." }
buildifier-lint = { bin = "buildifier", files = "glob:**/*.bzl,BUILD", args = "--lint" }
buildifier-fmt = { bin = "buildifier", files = "glob:**/*.bzl,BUILD" }

Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

Another round of mostly minor points, it looks good enough to land for me anyway.

cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Show resolved Hide resolved
cli/tests/test_fix_command.rs Outdated Show resolved Hide resolved
cli/tests/test_fix_command.rs Outdated Show resolved Hide resolved
cli/tests/test_fix_command.rs Outdated Show resolved Hide resolved
cli/tests/test_fix_command.rs Outdated Show resolved Hide resolved
cli/tests/test_fix_command.rs Show resolved Hide resolved
cli/tests/test_fix_command.rs Outdated Show resolved Hide resolved
cli/tests/test_fix_command.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
@hooper hooper force-pushed the push-yoyuqnpxtkql branch 2 times, most recently from ce622cc to d22a2ff Compare May 25, 2024 04:05
cli/src/commands/fix.rs Show resolved Hide resolved
cli/src/commands/fix.rs Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Show resolved Hide resolved
cli/testing/fake-formatter.rs Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/tests/test_fix_command.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
@hooper hooper force-pushed the push-yoyuqnpxtkql branch 2 times, most recently from dcab689 to 981555e Compare May 30, 2024 00:33
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

LGTM, if you create the issue and address my doc nits. I'd wait for a final approval from Waleed though, as he definitely is better qualified.

CHANGELOG.md Outdated Show resolved Hide resolved
cli/src/commands/fix.rs Outdated Show resolved Hide resolved
@hooper hooper force-pushed the push-yoyuqnpxtkql branch from 7fc5ce4 to a3dcdb4 Compare June 4, 2024 19:11
@hooper hooper merged commit 3050685 into main Jun 4, 2024
16 checks passed
@hooper hooper deleted the push-yoyuqnpxtkql branch June 4, 2024 19:28
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.

6 participants