Skip to content

switch block-style to line-style comments#770

Merged
davepacheco merged 1 commit into
mainfrom
normalize-comments
Mar 16, 2022
Merged

switch block-style to line-style comments#770
davepacheco merged 1 commit into
mainfrom
normalize-comments

Conversation

@davepacheco

@davepacheco davepacheco commented Mar 15, 2022

Copy link
Copy Markdown
Collaborator

(Disclaimer: this might be a bad idea or a bad time to do this. This experiment has taken about 5 minutes -- it's easy to throw out or try again later.)

Omicron currently has a mix of commenting styles -- both block style (C style, /* ... */) and line style (// ...). It seems like this has been a distraction for folks, and the line style is more popular, so I figured I'd try using rustfmt's [fraughtly-named] normalize_comments option to switch everything to line style. I've only barely spot-checked the results but it looks okay. I'll look a little closer if folks are on board with this.

If we decide to do this, I don't think we should enforce it with rustfmt because that requires an unstable option. We know what a pain that is with rustfmt. I don't expect this to be a big problem because I think line style tends to be the more common expectation.

If we decide to do this, it might be an annoying merge for anybody with outstanding work. Probably the best option would be to apply what I've done, which is:

$ git diff
diff --git a/rustfmt.toml b/rustfmt.toml
index 41d1d58b..a075ddd2 100644
--- a/rustfmt.toml
+++ b/rustfmt.toml
@@ -4,3 +4,7 @@
 max_width = 80
 use_small_heuristics = "max"
 edition = "2021"
+
+# Temp unstable features
+unstable_features = true
+normalize_comments = true

followed by cargo +nightly fmt. Do this before merging with this change and I'd expect the merge to be a no-op. I haven't tested this.

@david-crespo

david-crespo commented Mar 15, 2022

Copy link
Copy Markdown
Contributor

As I said in chat, I'm in favor of virtually any change that cuts 1200 lines with no change in functionality.

@smklein smklein mentioned this pull request Mar 16, 2022
Merged
@davepacheco davepacheco enabled auto-merge (squash) March 16, 2022 15:02
@davepacheco davepacheco disabled auto-merge March 16, 2022 15:03
@davepacheco davepacheco enabled auto-merge (squash) March 16, 2022 15:03
@davepacheco davepacheco changed the title switch block-style to line-style comments? switch block-style to line-style comments Mar 16, 2022
@davepacheco davepacheco disabled auto-merge March 16, 2022 16:03
@davepacheco davepacheco merged commit 8a26d69 into main Mar 16, 2022
@davepacheco davepacheco deleted the normalize-comments branch March 16, 2022 16:03
davepacheco added a commit that referenced this pull request Mar 16, 2022
@davepacheco
switch block-style to line-style comments (#770)
ea35b6e
davepacheco added a commit that referenced this pull request Mar 16, 2022
@davepacheco
premerge with #770
a036b21
@davepacheco

davepacheco commented Mar 16, 2022

Copy link
Copy Markdown
Collaborator Author

I ran into more trouble than I expected when merging. Here's what I'd suggest if you have outstanding Omicron PRs:

  1. First, merge with f7f3ff7, which is the last commit before this change.
  2. Copy this into rustfmt-new.toml:
# ---------------------------------------------------------------------------
# Stable features that we customize locally
# ---------------------------------------------------------------------------
max_width = 80
use_small_heuristics = "max"
edition = "2021"

# Temp unstable features
unstable_features = true
normalize_comments = true
  1. Make sure you have no uncommitted changes. Run cargo +nightly-2021-12-14 fmt -- --config-path=rustfmt-new.toml
  2. Run git status and eyeball the results -- should be 88 files changed, all Rust source files
  3. Run git commit -am 'premerge with #770'
  4. Merge with 8a26d6992dafea6f5052083b2362d13b8e94dfa6 (this change). This should pretty straightforward. If you have conflicts, they should be because you've changed comment lines. I think you pretty much want to take your version at this point (since step 3 will already have changed your comments to line-style).

For the record: there are two commits to main for this change:

commit 8a26d6992dafea6f5052083b2362d13b8e94dfa6 (HEAD -> main, origin/main, origin/HEAD)
Author: David Pacheco <dap@oxidecomputer.com>
Date:   Wed Mar 16 09:03:30 2022 -0700

    switch block-style to line-style comments (#770)

commit ea35b6ee1b15a7abff9fd690afd1090ab9121abc
Author: David Pacheco <dap@oxidecomputer.com>
Date:   Wed Mar 16 08:52:27 2022 -0700

    switch block-style to line-style comments (#770)

I had enabled "auto-merge" on this PR, and the checks completed around the time of ea35b6e. The UI reported that it was "attempting to auto-merge" for about 10 minutes before I gave up, disabled auto-merge, and hit "squash and merge" myself. It appears that the automerge had been successful, the UI didn't know that, and so it merged it again. 8a26d69 contains no changes.

@jgallagher jgallagher mentioned this pull request Mar 16, 2022
Merged
jmpesp added a commit to jmpesp/omicron that referenced this pull request Mar 18, 2022
@jmpesp
"cargo +nightly fmt" before pulling in main (to sync with oxidecomput…
b0e9474 
…er#770)
@davepacheco davepacheco mentioned this pull request Jan 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@davepacheco @david-crespo