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

switch block-style to line-style comments #553

Merged
merged 3 commits into from
Jan 9, 2023
Merged

switch block-style to line-style comments #553

merged 3 commits into from
Jan 9, 2023

Conversation

davepacheco
Copy link
Collaborator

This is modeled after oxidecomputer/omicron#770. Paraphrasing from there what applies here as well.


Dropshot 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. After the success of oxidecomputer/omicron#770 I used rustfmt's normalize_comments option to switch everything to line style. I don't think we should enforce this with rustfmt because that requires an unstable option, which we know from experience is painful. I don't expect this to be a big problem because I think line style tends to be the more common expectation.

When this lands, it may be annoying to merge with outstanding work. I'm happy to help with that. Based on last time, when this commit lands on "main", I think the right strategy is to sync up other PRs by:

  1. First merge with the parent commit of this commit on "main".
  2. Make sure you have no uncommitted local changes.
  3. Copy this to 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. Run cargo +nightly fmt -- --config-path=rustfmt-new.toml
  2. Run git commit -am 'premerge with style update'
  3. Merge with this commit on main. 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).

@davepacheco davepacheco requested a review from ahl January 9, 2023 21:30
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

checked everywhere for a similar problem but just found this one
git log --no-color -p main.. | grep '^-[^*]*$'

Comment on lines 190 to 206
/// #[dropshot::endpoint { method = GET, path = "/my/ws/endpoint/{id}" }]
/// async fn my_ws_endpoint(
/// rqctx: std::sync::Arc<dropshot::RequestContext<()>>,
/// websock: dropshot::WebsocketUpgrade,
/// id: dropshot::Path<String>,
/// ) -> dropshot::WebsocketEndpointResult {
/// let logger = rqctx.log.new(slog::o!());
/// websock.handle(move |upgraded| async move {
/// slog::info!(logger, "Entered handler for ID {}", id.into_inner());
/// use futures::stream::StreamExt;
/// let mut ws_stream = tokio_tungstenite::WebSocketStream::from_raw_socket(
/// upgraded.into_inner(), tokio_tungstenite::tungstenite::protocol::Role::Server, None
/// ).await;
/// slog::info!(logger, "Received from websocket: {:?}", ws_stream.next().await);
/// Ok(())
/// })
/// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

lost the formatting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great catch! For my future reference, I think it's because it wasn't formatted the way standard block-style comments are formatted (missing leading asterisks on those lines). I fixed this by reverting this file, fixing the C-style formatting, then rerunning cargo +nightly fmt -- --config-path=rustfmt-new.toml.

@davepacheco davepacheco enabled auto-merge (squash) January 9, 2023 22:24
@davepacheco davepacheco merged commit f3cf19e into main Jan 9, 2023
@davepacheco davepacheco deleted the style-update branch January 9, 2023 22:33
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.

3 participants