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

rustfmt for libgraphviz #29414

Closed
wants to merge 1 commit into from
Closed

rustfmt for libgraphviz #29414

wants to merge 1 commit into from

Conversation

scanfield
Copy link

Run rustfmt on libgraphviz

Tested: make -j8 check-stage1-std

Also ran rustfmt a second time to verify the transformation was stable.

Run rustfmt on libgraphviz

Tested: make -j8 check-stage1-std

Also ran rustfmt a second time to verify the transformation was stable.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@scanfield
Copy link
Author

r? @nrc

Not sure about the raw strings around line 1037 in the lhs -- do you have advice for that?

@rust-highfive rust-highfive assigned nrc and unassigned pnkfelix Oct 27, 2015
@@ -63,7 +63,8 @@
//! }
//!
//! impl<'a> dot::Labeller<'a, Nd, Ed> for Edges {
//! fn graph_id(&'a self) -> dot::Id<'a> { dot::Id::new("example1").unwrap() }
//! fn graph_id(&'a self) -> dot::Id<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't look right either

Copy link
Member

Choose a reason for hiding this comment

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

@nrc
Copy link
Member

nrc commented Nov 3, 2015

@stevencanfield thanks for the PR and sorry it took me a while to get to it (I've been travelling). There's a couple of good fixes here that I'd like to take, but the formatting of code in doc comments is a known problem with rustfmt and I think we should not land them. The raw string changes are interesting - I'm not sure if they are an improvement or not - do you have an opinion? I filed a rustfmt issue in any case. I was thinking that since the lines after the first are not indented, nor should the first be, but it is indented by the r" so there is no alignment anyway. @pnkfelix what do you think?

@bors
Copy link
Contributor

bors commented Nov 25, 2015

☔ The latest upstream changes (presumably #30017) made this pull request unmergeable. Please resolve the merge conflicts.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 3, 2015

sorry for my radio silence here. I'm fine with indenting the "first line" in a raw string (where if I understand correctly, the first line is the one that has the actual r"/r#"/r##"`/etc.)

(I suppose I'd probably opt for whatever the current emacs-mode does, which is probably a terrible way to make decisions. :) )

@pnkfelix
Copy link
Member

pnkfelix commented Dec 3, 2015

@nrc is #[rustfmt_skip] a known attribute to rustc at this point?

@nrc
Copy link
Member

nrc commented Dec 6, 2015

@pnkfelix sounds good. rustfmt_skip is not known to the compiler.

@stevencanfield sorry this PR got lost in review churn and thanks for contributing it. It needs a rebase, but I think some of these changes have landed in the meantime. So closing this PR.

@nrc nrc closed this Dec 6, 2015
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.

6 participants