Skip to content

doc: update instructions for installing from source #5755

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

Closed
wants to merge 1 commit into from

Conversation

xxchan
Copy link
Contributor

@xxchan xxchan commented Apr 24, 2023

Copied from Clippy doc

Copy link

@iamwacko iamwacko left a comment

Choose a reason for hiding this comment

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

All of the links work and there aren't any spelling or grammatical errors as far as I can see.

@calebcartwright
Copy link
Member

Thank you for the PR! I'll think on this some, but suspect it's unlikely we'll merge.

Installation is unnecessary (and discouraged) for rustfmt contributors, and strongly discouraged for users for many reasons. There's multiple reasons for this, not the least of which being that we sometimes break up larger changes into smaller PRs for easier review/discussion and using the interim, unreleased versions can fail to adhere to the formatting stability guarantee

@xxchan
Copy link
Contributor Author

xxchan commented Jun 19, 2023 via email

@calebcartwright
Copy link
Member

Again, thanks for taking the time to open a PR and sharing your perspective. However, I'm not keen on getting into a debate over this. I am similarly not going to take your bait around the current rustfmt volunteers being too slow for your liking, and not volunteering enough of our free time.

As the maintainers of the project, I think it's more than reasonable for us to have the final say on how we encourage, and discourage, users and potential contributors on something as foundational as installation.

Running a version of rustfmt from source code as part of a test or hacking on the rustfmt codebase is entirely possible and actually pretty easy. It's something those of us contributing regularly do quite commonly as part of our inner dev loop, and something that's also done on each and every CI run.

An example of one (of several) options using a basic cargo flow that I'd imagine most Rust devs are familiar with if they've ever worked with any type of bin target: cargo run --bin cargo-fmt -- --check --manifest-path path/to/project/you/want2test/Cargo.toml

Yes, I recognize that your proposed steps are useful instructions if someone is actually trying to install a version of rustfmt from source, but again my point is that it's our perspective that folks shouldn't be doing that regardless and we don't want to encourage it.

Overhauling the docs is one of the myriad things on my todo list, and part of that will include the removal, or at least removing the emphasis/prominence of the from-source installation steps. If that's something you'd be willing to submit a patch for then we can get that merged (and would be most appreciative!). However, I'm going to close this now as I don't want to move in this direction.

@xxchan
Copy link
Contributor Author

xxchan commented Jun 19, 2023 via email

@calebcartwright
Copy link
Member

Sure thing!

I’m not aware of the --manifest-path option and
accustomed to running cargo fmt without args in a workspace.

Yeah cargo fmt is significantly more ergonomic and we want it to be that way for end users, but the cargo run approach works pretty well and aliases can always make things easier to type 😄

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.

4 participants