-
Notifications
You must be signed in to change notification settings - Fork 40
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
switch block-style to line-style comments #770
Conversation
As I said in chat, I'm in favor of virtually any change that cuts 1200 lines with no change in functionality. |
7edbf9c
to
11679bc
Compare
I ran into more trouble than I expected when merging. Here's what I'd suggest if you have outstanding Omicron PRs:
For the record: there are two commits to
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. |
(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:
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.