Simple formatting of some modules #24927

Closed
wants to merge 8 commits into
from

Projects

None yet
@nrc
Contributor
nrc commented Apr 29, 2015

Done automatically with rustfmt.

@rust-highfive
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@bors
Contributor
bors commented Apr 30, 2015

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

@tbu- tbu- commented on the diff Apr 30, 2015
src/libgraphviz/lib.rs
@@ -531,29 +533,36 @@ pub enum RenderOption {
}
/// Returns vec holding all the default render options.
-pub fn default_options() -> Vec<RenderOption> { vec![] }
+pub fn default_options() -> Vec<RenderOption> {
+ vec![]
+}
@tbu-
tbu- Apr 30, 2015 Contributor

I think you should also support the style of function as it was before.

@pnkfelix
pnkfelix Apr 30, 2015 Member

I do like having the whole fn sig and body in one line when the body really is short. Is this hard to accommodate, @nrc ? Or is this more of a philosophical thing about "bodies should be presented separately from signatures" ?

@nrc
nrc Apr 30, 2015 Contributor

It's a bit of both tbh, I do prefer function bodies always having their own line - makes it easier to read and distinguish provided from required methods. It was also the easiest thing to do.

@tbu- tbu- commented on the diff Apr 30, 2015
src/libgraphviz/lib.rs
@@ -795,7 +806,8 @@ r#"digraph single_edge {
#[test]
fn test_some_labelled() {
let labels : Trivial = SomeNodesLabelled(vec![Some("A"), None]);
- let result = test_input(LabelledGraph::new("test_some_labelled", labels,
+ let result = test_input(LabelledGraph::new("test_some_labelled",
+ labels,
@tbu-
tbu- Apr 30, 2015 Contributor

Is this a rule that "either all parameters must be on their respective line, or they must share one"?

@pnkfelix
pnkfelix Apr 30, 2015 Member

(I'm fine with such a rule, personally.)

@nrc
nrc Apr 30, 2015 Contributor

yes

@tbu- tbu- commented on the diff Apr 30, 2015
src/librustc_resolve/lib.rs
@@ -68,7 +72,8 @@ use syntax::ast::{ExprLoop, ExprWhile, ExprMethodCall};
use syntax::ast::{ExprPath, ExprStruct, FnDecl};
use syntax::ast::{ForeignItemFn, ForeignItemStatic, Generics};
use syntax::ast::{Ident, ImplItem, Item, ItemConst, ItemEnum, ItemExternCrate};
-use syntax::ast::{ItemFn, ItemForeignMod, ItemImpl, ItemMac, ItemMod, ItemStatic, ItemDefaultImpl};
+use syntax::ast::{ItemFn, ItemForeignMod, ItemImpl, ItemMac, ItemMod,
+ ItemStatic, ItemDefaultImpl};
@tbu-
tbu- Apr 30, 2015 Contributor

Looking at the surrounding code, this is probably not wanted, but rather merging all these syntax::ast imports, sorting them and then splitting them into separate lines (with their own use syntax::ast::{}) according to line length.

@pnkfelix
pnkfelix Apr 30, 2015 Member

I don't know, reordering the input tokens is probably going too far, IMO ... usually pretty print / format tools just focus on adjusting whitespacing and line breaks.

(As another example, does rustfmt ever insert or delete trailing commas in argument/field lists? I actually do not know the answer)

((Update: Found my answer: there is an example of it removing a comma in one of the edits below. I do not know how I feel about that...))

@nrc
nrc Apr 30, 2015 Contributor

I intend for it to do what @tbu- wants - grouping and ungrouping imports. But that is pretty tricky to do. One day...

@pnkfelix I'm keen for rustfmt to do some quite advanced things - even more so than this kind of thing, e.g., convert glob imports to list imports. I hope that leaving these things as configurable rather than mandatory will assuage those who believe rustfmt should only change white space.

@Ryman
Ryman Apr 30, 2015 Contributor

As a comparison, gofmt reorders imports alphabetically. (If I recall correctly)

@slimsag
slimsag Apr 30, 2015

@Ryman It doesn't. goimports does but it's a separate tool.

@tbu- tbu- commented on the diff Apr 30, 2015
src/librustc_resolve/lib.rs
@@ -2194,7 +2177,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
// user and one 'x' came from the macro.
fn binding_mode_map(&mut self, pat: &Pat) -> BindingMap {
let mut result = HashMap::new();
- pat_bindings(&self.def_map, pat, |binding_mode, _id, sp, path1| {
+ pat_bindings(&self.def_map,
+ pat,
+ |binding_mode, _id, sp, path1| {
@tbu-
tbu- Apr 30, 2015 Contributor

I don't know what rules this follows, but in this specific example (and a few ones below), I think that the closure "header" should rather be on the same line as the other parameters.

@nrc
nrc Apr 30, 2015 Contributor

the rule here is each argument starts its own line and the closure body starts a new line. This comes from a rule which basically says that if any argument spans multiple lines, then each arg must be on its own line.

@tbu-
tbu- Apr 30, 2015 Contributor

I understand where this rule comes from, but I think the rules does not work well for functions taking closures, e. g. map_or, etc.

@bluss
bluss Apr 30, 2015 Contributor

You effectively read it as a line from pat_bindings( until the {. If you treat that as the function arguments, it's not longer than the text width here. I think that would work better. See also the example with walk_pat here just next hunk below.

@nrc
nrc Apr 30, 2015 Contributor

Hmm, I agree that the walk_pat formatting does not look great, and I imagine the same will happen for some uses of map_or. However, I do think in many cases (mostly without closures) having a sub-expression split vertically, but not stacking all args looks very odd. I wonder if the exception should be for closures? Or for the last argument maybe?

@nikomatsakis
nikomatsakis Jul 9, 2015 Contributor

I find closures as the last argument to follow a distinct rule...

@pnkfelix pnkfelix and 2 others commented on an outdated diff Apr 30, 2015
src/libgraphviz/lib.rs
/// Renders directed graph `g` into the writer `w` in DOT syntax.
/// (Simple wrapper around `render_opts` that passes a default set of options.)
-pub fn render<'a, N:Clone+'a, E:Clone+'a, G:Labeller<'a,N,E>+GraphWalk<'a,N,E>, W:Write>(
- g: &'a G,
- w: &mut W) -> io::Result<()> {
+pub fn render<'a, N: Clone+'a, E: Clone+'a, G: Labeller<'a, N, E>+GraphWalk<'a, N, E>, W: Write>
@pnkfelix
pnkfelix Apr 30, 2015 Member

man I totally have to rewrite this using a where clause some day. :)

(similarly below...)

@nrc
nrc Apr 30, 2015 Contributor

Give me a few more weeks and rustfmt will do it for you :-)

@erickt
erickt Apr 30, 2015 Contributor

Isn't our convention to have whitespace around trait constraints, as in N: Clone + 'a?

@nrc
nrc Apr 30, 2015 Contributor

The convention, as I observe it, is that we do in where clauses but not in bounds. I don't think this is a good state of affairs, but not adding spaces changed less code. It's easy to change.

@pnkfelix pnkfelix commented on the diff Apr 30, 2015
src/libgraphviz/lib.rs
@@ -822,10 +833,12 @@ r#"digraph single_cyclic_node {
#[test]
fn hasse_diagram() {
let labels = AllNodesLabelled(vec!("{x,y}", "{x}", "{y}", "{}"));
- let r = test_input(LabelledGraph::new(
- "hasse_diagram", labels,
- vec!(edge(0, 1, ""), edge(0, 2, ""),
- edge(1, 3, ""), edge(2, 3, ""))));
+ let r = test_input(LabelledGraph::new("hasse_diagram",
+ labels,
+ vec!(edge(0, 1, ""),
+ edge(0, 2, ""),
+ edge(1, 3, ""),
+ edge(2, 3, ""))));
@pnkfelix
pnkfelix Apr 30, 2015 Member

Hmm. I would have maybe hoped that we would support this:

+        let r = test_input(LabelledGraph::new(
+            "hasse_diagram",
+            labels,
+            vec!(edge(0, 1, ""), edge(0, 2, ""), edge(1, 3, ""), edge(2, 3, ""))));

That is, I think the idiom of putting a line break before the first argument to avoid the heavy indentation of the argument list is a pretty common idiom for us, no?

But maybe it is too hard for rustfmt to decide whether to adopt that strategy or not, since it is often subjective whether to employ it or not.

@nrc
nrc Apr 30, 2015 Contributor

Hmm, I'm not sure how to weight that decision - the heuristic I would use would try hanging indent if it means we can keep each argument on one line, which would address this case, I'm not sure if that is a good general rule.

I actually prefer the first layout and would use that if handwriting the code. But I have a stong aversion to hanging indents, I can see why you might prefer the latter.

@pnkfelix
pnkfelix Apr 30, 2015 Member

So on further reflection, how about: Try hanging indent if it means we can keep all the arguments on the same line?

(It won't address this case, but that is okay with me.)

Of course, for all I know my suggestion is what rustfmt already does ...

@nikomatsakis
nikomatsakis Jul 9, 2015 Contributor

My 2 cents: I find it depends on the case, sometimes I prefer hanging indent, sometimes not, depending on various concerns. I think it'd be good to preserve what the author chose. I really dislike when all the code is bunched up against the right margin though, so I tend to prefer hanging idents if that's the only way to get "full" utilization.

@pnkfelix pnkfelix commented on the diff Apr 30, 2015
src/librustc_resolve/lib.rs
@@ -2677,8 +2665,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
path: &Path,
namespace: Namespace,
check_ribs: bool)
- -> AssocItemResolveResult
- {
+ -> AssocItemResolveResult {
@pnkfelix
pnkfelix Apr 30, 2015 Member

hmm I guess I really am going to have to go fix those emacs rust-mode indentation bugs that have led me to put in these line breaks before the {

@nrc
nrc Apr 30, 2015 Contributor

I actually prefer the linebreak before the { too, and have built in an option to do it in rustfmt. I feel like I should make an RFC for the change to the style guide or something though.

@tikue
tikue Apr 30, 2015

👍 breaking before { is really good when a where clause exists, and further, I think it makes sense to be consistent and just do it everywhere.

@pnkfelix
Member

Overall this looks reasonable. There were some nits noted above.

@nrc maybe you should do another pass with your fixed rustfmt and force-push that, so that we can clearly distinguish the manual fixups to graphviz from the fixes to rustfmt? In particular some of the previous deltas were clear bugs, and I want to just double-check that those were indeed fixed.

@bluss bluss commented on the diff Apr 30, 2015
src/librustc_resolve/lib.rs
@@ -3049,8 +3037,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}
}
- fn with_no_errors<T, F>(&mut self, f: F) -> T where
- F: FnOnce(&mut Resolver) -> T,
+ fn with_no_errors<T, F>(&mut self, f: F) -> T
+ where F: FnOnce(&mut Resolver) -> T
@bluss
bluss Apr 30, 2015 Contributor

I don't like this where placement. It's doesn't scale well to multiple lines in the where clause.

    fn with_no_errors<T, F>(&mut self, f: F) -> T where
        F: FnOnce(&mut Resolver) -> T,
        T: MoreBoundsHere,

Mostly the question is: What does the more complicated where clause look like. Bunched up on one line the docs is not acceptable, we need readable & diffable :-)

@nrc
nrc Apr 30, 2015 Contributor

It scales to:

fn with_no_errors<T, F>(&mut self, f: F) -> T where
    where F: FnOnce(&mut Resolver) -> T,
          T: MoreBoundsHere,

I find that easier to scan because you see the where keyword when scanning down the lhs

@bluss
bluss Apr 30, 2015 Contributor

I think it's ok.

Are we free to choose any style even if dumb editors may make it hard to replicate? I guess rustfmt can fix it. I'm sure I can get Vim to do this style, but it's a tedious style to space out manually for those with dumber editors or those that just don't install rust plugins.

@erickt erickt commented on the diff Apr 30, 2015
src/libgraphviz/lib.rs
-pub fn render_opts<'a, N:Clone+'a, E:Clone+'a, G:Labeller<'a,N,E>+GraphWalk<'a,N,E>, W:Write>(
- g: &'a G,
- w: &mut W,
- options: &[RenderOption]) -> io::Result<()>
-{
- fn writeln<W:Write>(w: &mut W, arg: &[&str]) -> io::Result<()> {
+pub fn render_opts<'a,
+ N: Clone+'a,
+ E: Clone+'a,
+ G: Labeller<'a, N, E>+GraphWalk<'a, N, E>,
+ W: Write>
+ (g: &'a G,
+ w: &mut W,
+ options: &[RenderOption])
+ -> io::Result<()> {
+ fn writeln<W: Write>(w: &mut W, arg: &[&str]) -> io::Result<()> {
@erickt
erickt Apr 30, 2015 Contributor

Would it be possible to better distinguish between the function arguments and body? They blend together visually for me. My personal style is:

pub fn render_opts<
    'a,
    N: Clone+'a,
    E: Clone+'a,
    G: Labeller<'a, N, E>+GraphWalk<'a, N, E>,
    W: Write,
>(
    g: &'a G,
    w: &mut W,
    options: &[RenderOption],
) -> io::Result<()> {
    fn writeln<W: Write>(w: &mut W, arg: &[&str]) -> io::Result<()> {

To make it a little closer aligned to this, it would at least be nice if the argument list is long that the { is bumped to the next line, a la:

pub fn render_opts<'a,
                   N: Clone+'a,
                   E: Clone+'a,
                   G: Labeller<'a, N, E>+GraphWalk<'a, N, E>,
                   W: Write>
    (g: &'a G,
     w: &mut W,
     options: &[RenderOption])
     -> io::Result<()>
{
    fn writeln<W: Write>(w: &mut W, arg: &[&str]) -> io::Result<()> {
@nrc
nrc Apr 30, 2015 Contributor

rustfmt currently only does newline brace if there is a where clause, I would like to do it everywhere, but that is a big style change. I guess bumping it if it is a mulit-line sig is a compromise, but it seems somewhat inconsistent to me.

@nrc
Contributor
nrc commented May 1, 2015

@pnkfelix rebased and, as suggested, manual fixups separated out better

@MDCox MDCox commented on the diff May 1, 2015
src/libgraphviz/lib.rs
EscStr(s.into_cow())
}
- fn escape_char<F>(c: char, mut f: F) where F: FnMut(char) {
+ fn escape_char<F>(c: char, mut f: F)
+ where F: FnMut(char)
+ {
@MDCox
MDCox May 1, 2015 Contributor

This feels strange since the curly braces normally open at the end of the function signature, but here it's on it's own line. I understand why it's done, but there's a definite aesthetic dissonance.

@bors
Contributor
bors commented May 12, 2015

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

@alexcrichton
Member

Closing due to inactivity, but looking forward to possibly using rustfmt in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment