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

Define the maximum width of a line and the width of indentation. #2

Closed
wants to merge 1 commit into from

Conversation

nrc
Copy link
Member

@nrc nrc commented May 4, 2016

Closes #1

@joshtriplett
Copy link
Member

This seems reasonable, but it needs more detail in several areas.

Most importantly, we need to define what to do in cases where wrapping at 100 must not happen. For example, taking inspiration from the coding style guidelines for the Linux kernel, I would advocate not breaking any string constant intended for printing to humans into multiple concatenated constants for word-wrapping; otherwise, someone searching for an error message in the code may not find it. (Wrapping immediately after a '\n' works, though.) Given this, though, rustfmt also shouldn't do silly things like wrapping a ; or ); immediately after the string constant onto another line.

Similarly, within a comment, a fixed-width block shouldn't get word-wrapped automatically. For instance, if someone puts a carefully formatted ASCII-art table or diagram into their comment, with a width of 120 characters, rustfmt should not mangle that. (It would presumably need to appear between triple-backquotes or similar.)

Finally, I think this needs associated reasonable procedures for where to break lines, how to indent continuation lines, and how to align those continuation lines. Those procedures might not need to appear within this RFC, but this RFC should acknowledge the need for them, and rustfmt shouldn't enforce wrapping (or un-wrapping) if it doesn't have a good way to handle alignment. (Simple example: whether to align operands of a binary operator, and whether to wrap before or after the operator. Another example: when wrapping a long if or while condition, where should the continuation line get indented to, and will it avoid potential confusion with the following body lines?)

@nrc
Copy link
Member Author

nrc commented Aug 25, 2016

@joshtriplett good points, and I think these should be addressed.

In the meantime, just a note that this RFC PR should probably be re-written once we've had some more feedback on #1 and the style team agrees on the RFC format. This PR was made more as an example of what a style RFC might look like, rather than as an actual RFC to discuss and accept.

I'll leave it open for now, since I think it is useful to have an example and because I don't want to forget about @joshtriplett's comment, but I intend to close it in due course.

@joshtriplett
Copy link
Member

Ah, I didn't realize this was intended more as a sample. I probably should have posted the above comment to #1 then.

@joshtriplett joshtriplett mentioned this pull request Aug 25, 2016
@brson
Copy link

brson commented Sep 2, 2016

I'm not sure I understand what "A comment should be wrapped at the minimum of the
80 character comment limit and the 100 character line limit" means. Since the minimum of 80 and 100 is 80, it seems to say "a comment should be wrapped at 80 characters".

Personally I'm fine with these rules. I think the rationale for 100-char lines and 80-char comments is as reasonable as might be expected; though even though I advocated the 100 character limit I do find myself frequently annoyed at long-lines on my split-screen 13-inch laptop.

I know there's a vocal contingency that thinks 2-space indent is correct for Rust, and that would ease the need for 100-char lines. I do think it's tempting to take this (probably last) opportunity to switch to 2-spaces/80-chars. It may be worth reformatting all of Rust with these settings to take a look at something concrete.

cc @wycats @alexcrichton.

[summary]: #summary

Define the maximum width of a line and the width of indentation.

Copy link
Member

Choose a reason for hiding this comment

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

✂️ (extra \n here)

@steveklabnik
Copy link
Member

Yes, to echo what @brson said, there's two camps:

  • 100 columns, four space indent
  • 80 columsn, two space indent

The first was semi-arbitrarily chosen a while back, and is what most things do today. However, as I mentioned inline, the standard library seems to often do four space indent, 80 cols, which is the worst of both worlds.

Ruby uses style number two, so I'm happy with it, but I've mostly been using style number one, since it was the largest contingent of people, and I'm happy with it as well. To me, this is the most core question: which of these two options should it be? I'm not totally sure about making comments different from other kinds of code; I'd prefer the simplicity of one style for everything.

@joshtriplett
Copy link
Member

joshtriplett commented Sep 2, 2016

(Obligatory acknowledgement that this is the bikesheddiest of all bikeshed topics.)

@steveklabnik
I agree that comments shouldn't follow different rules.

I've become increasingly convinced, however, that we shouldn't have a fixed width at all. I do think we should break lines in logical locations, but I don't think I've ever seen a case where a line break that made sense with a line width of 80 wouldn't make just as much sense to preserve with a line width of 500. For instance, if you have an expression like this:

if complex_expression(x)
   || complex_expression(y)
   || complex_expression(z) {

I'd suggest wrapping it that way no matter how the width of complex_expression compares to the target line width. Even if you have a line width of 500, I wouldn't put all that on one line, because wrapping it that way shows the similarity (complex_expression) and differences (x, y, z) between the lines.

Conversely, for an expression like this:

                        if an_interesting_function(x) == TARGET_VALUE_FOR_X
                           || an_interesting_function(y) == TARGET_VALUE_FOR_Y {

I can't imagine a scenario where that code becomes clearer by putting a line break next to the ==.

Likewise, no matter how far to the right your indentation gets, I don't think it makes sense to wrap println!("{}", x); onto two lines (the second containing x);).

Unfortunately, the concept of "logical line breaks" makes an automated formatting tool like rustfmt much more complex.

@alexcrichton
Copy link
Member

Thanks for the cc @brson!

I personally fall very squarely into the "80 column hard limit" camp. I personally work on a relatively small screen (13" laptop) where once I have 100 columns of code on my screen it ends up taking quite a bit of screen real-estate. Additionally, I've found that 80 columns works very well for basically all other tools in existence: diff, github, pastes, browser windows, etc. All that's really just a long winded way of saying that I believe I'm actively more productive with 80 characters as in the long run I can just see more on a screen. I'm sure others feel differently though!

I also agree with @steveklabnik that there are two camps here where the column limit is closely related to the tab number as well. I personally have no opinions on this in the sense that I'm perfectly fine with either 2 or 4 space tabs plus and 80 column limit.

As @steveklabnik also mentioned the prevailing style of the standard library is 80 columns, but I don't think this is consistently applied (or in the compiler as well). I think it may be a data point for what the style would look like, but I wouldn't necessarily hold it as "precedent to make a decision one way or another".

Now finally, I'll agree with @joshtriplett that this is indeed one of the most bike-sheddy topics available! Along those lines I don't want to represent a hard stance or anything, just offer my opinion. Whatever we choose I'll learn to deal with!

80 character comment limit and the 100 character line limit.

Indentation should use spaces, not tabs. Each level of indentation should be
four characters wide.
Copy link
Member

@solson solson Sep 2, 2016

Choose a reason for hiding this comment

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

I know there's a sizable minority that wishes we would switch to two spaces because of Rust's tendency to have a lot of indentation levels (rightwards drift). I'm not sure how the wider community feels about that.

EDIT: Oops, should have read the out-of-line comments first. I guess we'll get around to discussing this again on a later PR.

@nikomatsakis
Copy link

One thing about rustfmt today is that it sometimes fails to achieve the limit you request of it. I'm not sure of the precise details about when it will do this, but I find it interesting -- would we want to impose a stricter limit (like, e.g., the compiler does) even if rustfmt can't fully automate it? I kind of want to kill tidy and just say that "if rustfmt outputs it, good enough".

@aturon
Copy link
Member

aturon commented Sep 2, 2016

Comments are accumulating here pretty quickly, which is a little worrisome if this is supposed to be a "test" RFC. Can I suggest somebody on the team close this out in favor of wherever you'd prefer to have the discussion?

@wycats
Copy link

wycats commented Sep 2, 2016

I'm a strong proponent of 2-space tabs. I am very interested in hearing from someone who believes we should standardize on 4-space tabs for some reason other than inertia and the status-quo. I don't totally discount the status quo argument, and am prepared to debate it, but first I'd like to identify whether there are any strong a priori arguments for 4-space tabs.

The TLDR of my position in favor of 2-space tabs is that Rust has a number of common constructs that produce indents (impl, fn, match, match arms) and also tends to have long function signatures, which necessitate some nesting of multi-line function signatures. Nested functions create an especially bad combination of rightward-drift-inducing features. In practice, 4-space tabs results in a fair bit of rightward drift and, to me at least, makes poor use of the available horizontal space.

Informal polling has revealed far more support for 2-space tabs than I initially expected (please do speak up if you're in favor), which has given me some impetus to argue that we should consider standardizing around them.

@aturon
Copy link
Member

aturon commented Sep 2, 2016

FWIW I'm definitely interested in exploring 2-space tabs. This is our golden opportunity to do so, and there could be significant benefit.

I wouldn't say I'm a strong proponent -- at least, not yet -- but that's mostly because I haven't thought carefully through it. Like @wycats I'm curious to hear the fundamental arguments for larger widths.

@nikomatsakis
Copy link

nikomatsakis commented Sep 2, 2016

If we take status quo out of the equation, I am mildly in favor of 2-space tabs. Basically any time I have made the shift, I have found I like it better. It makes things like match in particular somewhat nicer. If nothing else, when I am not in a convenient editor I don't have to slam the space bar so many times. :)

I imagine the a priori argument in favor of 4-space is that it helps to avoid deep nesting. I do find my brain has a rather low "max recursion depth" and can get confused by deeply nested expressions and code. But I am not sure that the number of spaces used in indenting is the major contributing factor here.

I have no strong opinion about 80 vs 100 characters, but agree we should settle on one of those two. In the past Rust had a 78 character limit which I found quite annoying, but that was with 4-space tabs, and it was also without any kind of automated support for meeting that limit -- so mostly you only noticed it when make tidy ran, very late in the process.

@wycats
Copy link

wycats commented Sep 2, 2016

I do find my brain has a rather low "max recursion depth" and can get confused by deeply nested expressions and code. But I am not sure that the number of spaces used in indenting is the major contributing factor here.

If we want to encourage this, we could create a complexity or max-recursion lint that would target this problem more directly. I think the four-space syntactic vinegar is too bitter for "ok" cases to justify its impact on "problematic" cases, especially when more narrowly tailored solutions to that problem are available.

@nikomatsakis
Copy link

OK, I now saw @nrc's request to move these comments to #1 :)

@nrc
Copy link
Member Author

nrc commented Sep 2, 2016

In the meantime, just a note that this RFC PR should probably be re-written once we've had some more feedback on #1 and the style team agrees on the RFC format. This PR was made more as an example of what a style RFC might look like, rather than as an actual RFC to discuss and accept.

I'll leave it open for now, since I think it is useful to have an example and because I don't want to forget about @joshtriplett's comment, but I intend to close it in due course.

@nrc nrc closed this Sep 2, 2016
@wycats
Copy link

wycats commented Sep 2, 2016

@nrc would you mind summarizing and linking to the salient arguments people wrote in this thread to #1? Or would you prefer us to copy-paste the comments into #1?

@nrc
Copy link
Member Author

nrc commented Sep 3, 2016

@wycats I'll summarise on #1

@nrc nrc mentioned this pull request Sep 15, 2016
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.

None yet

9 participants