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

Format diff line to be easily clickable #5971

Merged
merged 1 commit into from Jan 25, 2024

Conversation

joshka
Copy link
Contributor

@joshka joshka commented Dec 1, 2023

Instead of {filename} at {line}, use {filename}:{line} as this is a
format that many editors allow to be clicked to navigate directly to the
line.


This is a small papercut issue. I like being able to Cmd+Click a filename and navigate directly to the problematic code when I'm running rustfmt check.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 25, 2024

Thanks! I've tested this out in VSCode and it works! Just wondering, does it make sense to leave the trailing colon :?

@joshka
Copy link
Contributor Author

joshka commented Jan 25, 2024

Thanks! I've tested this out in VSCode and it works! Just wondering, does it make sense to leave the trailing colon :?

I can take it or leave it on that. The colon indicates that this message belongs to the following error, so I think it's worthwhile. That said, build errors from cargo build / rustc don't have the colon (but they do have some shiny colors to highlight the line with the clickable file link.

The other thing worth considering here is whether to drop the full path and just leave the project root path e.g. compare:

[cargo-make] INFO - Execute Command: "rustup" "run" "nightly" "cargo" "fmt" "--all" "--check"
Diff in /Users/joshka/local/ratatui/src/backend/crossterm.rs at line 17:
 };
...

vs:

   Compiling ratatui v0.25.0 (/Users/joshka/local/ratatui)
error[E0433]: failed to resolve: use of undeclared crate or module `scrate`
  --> src/backend/crossterm.rs:19:5

You might also consider changing the message to be more explicit about what the diff represents to make it clearer to newer contributors when the format check is called deep in some CI process.

e.g. something like:

error: Formatting check failed at src/backend/crossterm.rs:17. Diff:
...
run `<full command line without the --check>` to fix these changes

@ytmimi
Copy link
Contributor

ytmimi commented Jan 25, 2024

I can take it or leave it on that. The colon indicates that this message belongs to the following error, so I think it's worthwhile.

Okay, cool. I'm fine with leaving it 👍🏼

but they do have some shiny colors to highlight the line with the clickable file link

We produce colored output for our internal errors, when you have the right setting enabled. To test that out you can run

rustfmt --config=max_width=15,error_on_line_overflow=true <<< 'fn main() { println!("hello world!"); }'
Screen Shot 2024-01-25 at 11 10 23 AM

The other thing worth considering here is whether to drop the full path

I've thought about that before. I think it's a chance I'd like to make a some point, but not in this PR.

You might also consider changing the message to be more explicit about what the diff represents to make it clearer to newer contributors when the format check is called deep in some CI process.

Thank you for the suggestion. I'll think on it. For now I'd like to keep the message as is.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

After our discussion I'm feeling good about making this change.

@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Jan 25, 2024
Instead of {filename} at {line}, use {filename}:{line} as this is a
format that many editors allow to be clicked to navigate directly to the
line.
@ytmimi ytmimi merged commit 7f44a08 into rust-lang:master Jan 25, 2024
27 checks passed
@joshka joshka deleted the format-diff-filename branch January 25, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Needs an associated changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants