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 placeholder code when generating a crate #7827

Conversation

Kinrany
Copy link
Contributor

@Kinrany Kinrany commented Jan 25, 2020

When generating source files in cargo new and cargo init, try to run rustfmt CLI on them.

If it fails, log the error and proceed.

Tests:

  • Works for cargo init --lib
  • No changes in behavior if rustfmt is missing

Also install rustfmt component in CI.

Also replace clippy_is_available with a more general command_is_available.

Closes #7656

Today I learned that `rustfmt` is now a part of `rustup`. So I need
to actually look it up in PATH instead of using `cargo-fmt`.

I also learned that `mk` is the file that does the main work and is
shared by `cargo-new` and `cargo-init`, as expected.

Printing inside `mk` produces output as expected.
I think I found the correct place in the algorithm where rustfmt should
be called.

I'm not sure if using `std::process::Command` and converting errors into
warnings is the correct way here.

Also the warning should only be printed once. Unless
`config.shell().warn()` deduplicates warnings, right now there will be
one warning per source file.

So I should probably check that `rustfmt` exists only once and disable
formatting if it doesn't.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2020
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 25, 2020

It looks good to me, can we add a test?

@Kinrany
Copy link
Contributor Author

Kinrany commented Jan 25, 2020

It looks good to me, can we add a test?

Sure! One for the good case and one for the case with missing rustfmt

Should I put them in tests/testsuite/new.rs and/or tests/testsuite/init.rs?

I actually wasn't planning on submitting this PR just yet, which is why it's a draft and there's no description. I'll make sure it only prints the warning once and reword the warning first.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 25, 2020

Either location would be great. Let me know when you are ready for revue.

src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
There's now exactly one warning per new crate instead of one per
source file.

Also warnings are being logged now, not printed to console.
Test 1: init a library crate with a `rustfmt.toml` config file in it.
Expect the generated source files to be formatted according to the
config file.

Test 2: same as test 1, but with missing `rustfmt`. Expect `cargo init`
to ignore the absence of `rustfmt` and generate source files
with default formatting.
@Kinrany Kinrany marked this pull request as ready for review January 27, 2020 16:50
@Kinrany
Copy link
Contributor Author

Kinrany commented Jan 27, 2020

@Eh2406 ready 😃

Either location would be great. Let me know when you are ready for revue.

I eventually decided to put them in init.rs since testing init seems simpler than using cargo new with a workspace.

@Kinrany
Copy link
Contributor Author

Kinrany commented Jan 27, 2020

Also feel free to bikeshed: I'm sure this is not entirely idiomatic Rust

@ehuss
Copy link
Contributor

ehuss commented Jan 27, 2020

I would leave it how it was before and avoid calling rustfmt twice. You can just log the error instead of using ?. Also, it might be helpful for debugging purposes to check if running rustfmt fails and log the output if it does. As it is, it is being captured and then discarded.

Also, the tests appear to be failing.

@Kinrany Kinrany force-pushed the 7656-format-placeholder-code-when-generating-a-crate branch from ae4adb2 to ea4f53c Compare January 27, 2020 21:46
The tests were failing with error:
"could not determine the current user, please set $USER"
@Kinrany
Copy link
Contributor Author

Kinrany commented Jan 27, 2020

Oops about the tests, I'm still on Windows. Should be fixed now.

Edit: nope, not fixed.

I would leave it how it was before and avoid calling rustfmt twice.

The source files are being created in a loop, so it's possible that rustfmt will be called multiple times, isn't it?

@ehuss
Copy link
Contributor

ehuss commented Jan 27, 2020

The source files are being created in a loop, so it's possible that rustfmt will be called multiple times, isn't it?

Hm, I forgot that cargo init can work with pre-existing sources. That makes me more nervous to have this change apply to cargo init. Imagine someone who doesn't want to use rustup, and runs it against their source, it would (silently) modify it with no means to revert. It seems like it would be much safer to only run rustfmt on new files.

I wouldn't worry about the performance if rustfmt isn't installed. Code simplicity is more important here.

@Kinrany
Copy link
Contributor Author

Kinrany commented Jan 28, 2020

It seems like it would be much safer to only run rustfmt on new files.

Yeah, I initially considered formatting the whole directory. I think I'm only applying rustfmt to files that were created two lines earlier, so it should be fine?

I wouldn't worry about the performance if rustfmt isn't installed. Code simplicity is more important here.

I was mostly worried about spamming the same error multiple times. But I guess this is less of a problem with logs, will fix.

Worst case is that the logs will have multiple errors caused by missing
rustfmt, so checking separately that rustfmt exists is unnecessary.
@Kinrany
Copy link
Contributor Author

Kinrany commented Jan 30, 2020

I think the test (formats_source) is failing because those environments do not have rustfmt. "Linux (stable)" passes, and I was unable to reproduce the "Linux (beta)" failure on a new Ubuntu 18 LTS.

Does it make sense to just disable the test if there's no rustfmt?

This could also be a problem for other contributors. But I could add a note that this error can be safely ignored if the user doesn't have rustup/rustfmt.

@ehuss
Copy link
Contributor

ehuss commented Feb 4, 2020

Sorry for the delay. Can you add rustup component add rustfmt to the CI script? I think it would go around here.

@Kinrany
Copy link
Contributor Author

Kinrany commented Feb 4, 2020

Oh, I totally should have thought of doing that D:

Done!

Generalized `clippy_is_available` and renamed as `command_is_available`.

No checks in `ignores_failure_to_format_source`, it's not supposed to
use `rustfmt` even if it's available
@ehuss
Copy link
Contributor

ehuss commented Feb 5, 2020

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 5, 2020

📌 Commit bc4c65c has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2020
@bors
Copy link
Contributor

bors commented Feb 5, 2020

⌛ Testing commit bc4c65c with merge db0dac5...

bors added a commit that referenced this pull request Feb 5, 2020
…ating-a-crate, r=ehuss

Format placeholder code when generating a crate

When generating source files in `cargo new` and `cargo init`, try to run `rustfmt` CLI on them.

If it fails, log the error and proceed.

Tests:
* Works for `cargo init --lib`
* No changes in behavior if `rustfmt` is missing

Closes #7656
@bors
Copy link
Contributor

bors commented Feb 5, 2020

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing db0dac5 to master...

@bors bors merged commit bc4c65c into rust-lang:master Feb 5, 2020
@Kinrany
Copy link
Contributor Author

Kinrany commented Feb 5, 2020

Yay, thanks :D 🎉

@ehuss ehuss added this to the 1.43.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format placeholder code when generating a crate
5 participants