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 all rust files #421

Closed
wants to merge 6 commits into from
Closed

Conversation

meltinglava
Copy link
Contributor

No description provided.

@phimuemue
Copy link
Member

I think it is a good idea to try to settle the formatting issue "once and for all" (even if it leads to conflicts in existing PRs (many of them have conflicts anyway)), as we had some commits that were accidentally introducing quite some formatting changes.

Maybe we should just add how we formatted them, or provide a clippy.toml.

@orium
Copy link
Contributor

orium commented Mar 10, 2020

I would add a cargo fmt -- --check to travis to make sure no one breaks it.

@meltinglava
Copy link
Contributor Author

should probably squash all the commits in this PR on merge

@phimuemue
Copy link
Member

I would add a cargo fmt -- --check to travis to make sure no one breaks it.

I am skeptical: Do we really want to reject pull requests because of (basically) whitespace issues? If so, shouldn't we check as early as possible (e.g. somewhen pre-committish)?

Do we know that cargo fmt works the same for all rust versions used by contributors?

Is there a bot that applies cargo fmt after a pull request is merged (possibly creating an own commit for this)? This would continuously format the codebase, and still allow contributors to not care too much about formatting.

@orium
Copy link
Contributor

orium commented Mar 10, 2020

I would add a cargo fmt -- --check to travis to make sure no one breaks it.

I am skeptical: Do we really want to reject pull requests because of (basically) whitespace issues?

I would say so. No one wants trailing whitespace on the code, nor an inconsistent format style.

Do we know that cargo fmt works the same for all rust versions used by contributors?

rustfmt has some strict stabilization promises, although some limitation apply. Also, this only holds true in versions ≥ 1.0 (but 1.0 is pretty old by now: Nov 19, 2018).

There are, however, features we are using in rustfmt.toml that got stabilized latter, so there is effectively a minimum rustfmt version to use, but I think that's fine.

@meltinglava
Copy link
Contributor Author

meltinglava commented Mar 10, 2020

Is there a bot that applies cargo fmt after a pull request is merged (possibly creating an own commit for this)? This would continuously format the codebase, and still allow contributors to not care too much about formatting.

I like this idea

Do we know that cargo fmt works the same for all rust versions used by contributors?

thats whats rustfmt.toml does

@meltinglava
Copy link
Contributor Author

TODO: have rustfmt only be run on stable or nightly

@jswrenn
Copy link
Member

jswrenn commented Mar 13, 2020

I would add a cargo fmt -- --check to travis to make sure no one breaks it.

I am skeptical: Do we really want to reject pull requests because of (basically) whitespace issues?

I would say so. No one wants trailing whitespace on the code, nor an inconsistent format style.

I'm really reluctant for itertools to adopt a viral formatting policy. I would much prefer PR authors adhere to the good practice of submitting minimal patches, than to force everyone to use rustfmt. I don't see evidence that inconsistent formatting is such an issue in the itertools codebase, we need to block PRs on, say, an extra space after the ! in a macro invocation.

A bot that automatically applies formatting to PRs would be welcome.

I'll (reluctantly) merge a mass formatting PR that doesn't change CI.

@meltinglava
Copy link
Contributor Author

I am skeptical: Do we really want to reject pull requests because of (basically) whitespace issues? If so, shouldn't we check as early as possible (e.g. somewhen pre-committish)?

This PR started when I was working on #363. As I have autoformat on save, and it formated src/lib.rs. That makes a big change to the file and I did not notice on the commit. This is just as good reason to "reject" a RP. I find it easier to work on a repo that has a formating style. This PR is more to get cargo fmt to work the same way for everyone and to stop blockers like this.

The only thing here is that its to run a program (cargo fmt). Its not manual labor. I am also willing to rebase current PR-s to make them adhere to master.

Also if the style does not adhere the reviewer can easily just add that patch, or just push the changes if the user has put Allow edits from maintainers is enabled.

If I was told in a review something like. Hey, the formating check failed. Can you run cargo fmt and then commit that. (it will only change their code if all of the code adheeres to the formating in the first place).

Also we might want to make a pre-commit file for those who are useing it.

I do not have the knowlage to create fix fmt from a bot. Thow I think this one can be merged (once I fix running fmt on one target) and add that bot in the future

@ChiefMilesEdgeworth
Copy link

+1 to applying fmt automatically if possible. I'm new-ish to the rust community, and it's really nice when code-bases are formatted because I can trust that I don't have to worry about style. I can contribute without worrying about formatting, and then just run cargo fmt or have an IDE format the file for me, and it won't introduce a bunch of work for the reviewer.

@jswrenn
Copy link
Member

jswrenn commented Mar 1, 2021

I'm going to close this PR, but I promise I'll revisit reformatting itertools in the not-too-distant future. My free time is extremely limited right now (I'm wrapping up my PhD), so when it comes to itertools, my priority is reducing any friction that would keep me from reviewing and merging PRs whenever I can find time.

There are two sources of friction here:

  • Merging repository-wide rustfmt is going to introduce merge conflicts into a lot of outstanding PRs, introducing friction for both me and the authors of those PRs.
  • Reviewing a repository-wide rustfmt is lot of avoidable work: it's easier for me to just run rustfmt myself and merge that, then to confirm that a formatting PR is only formatting.

So, when I do move forward with this, it will probably be:

  • something done on a per-file basis, for all files not touched by an outstanding PR (i'll gradually decrease the scope of excluded files)
  • something I'll do myself, rather than third-party PR (sorry, it just saves a bit of time that way)

In the meantime, the implications for would-be contributors are:

  • that I won't be very demanding about how contributors format their code (just try to stick to four spaces for indents?)
  • that contributors should try to be cognizant about not introducing sweeping formatting changes unrelated to their PR. I apologize for the inconvenience this can cause with editors configured to auto-format on save. (Disable automatic formatting for nightly users #395 will mitigate some of that pain, I hope.)

@jswrenn jswrenn closed this Mar 1, 2021
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.

5 participants