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

Add absolute_comment_width setting #2076

Conversation

matthew-mcallister
Copy link
Contributor

Some coding standards, such as PEP8, require wrapping block comments at a certain column number rather than a certain length from the start of the comment, which improves readability on narrow displays. Here is an option to make comment_width behave as a fixed column number rather than a relative width.

@nrc
Copy link
Member

nrc commented Oct 26, 2017

Thanks for the PR! I'm not sure if this change is that useful, however. Given that comments are limited by the max_width, is it common to want to limit them to a different absolute width?

(As an aside, I think that being able to implement coding standards other than the Rust style (e.g., PEP8) is a 'nice to have' but not a core goal for Rustfmt. I appreciate that this is a small change, but I feel like we have too many options right now, so I'm wary of adding another).

@matthew-mcallister
Copy link
Contributor Author

matthew-mcallister commented Oct 26, 2017

Well, ultimately this should be decided by others, but let me make my case for why this is good behavior. First, to clarify, I pointed to PEP8 to demonstrate that this is a very widely followed convention, and a good one I think, not because my goal is to arbitrarily support features of another coding standard. In fact, at first I thought rustfmt's comment wrapping was bugged; I've never even heard of anyone following the convention set by the default behavior.

Now, if you haven't before, you might consider reading PEP8's section on line length. PEP8 generally does a very good job justifying its choice of conventions; here's a relevant excerpt:

For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters.

Limiting the required editor window width makes it possible to have several files open side-by-side, and works well when using code review tools that present the two versions in adjacent columns.

Numbers aside, the two key points here are:

  1. Comments do not have the same structural restrictions as code; hence, it is easy to wrap them at a short line width.
  2. One of the main goals of line wrapping is making code readable in narrow displays, such as side-by-side columns.

Considering this, it seems desirable to prevent comments from extend past a certain absolute width determined by the concerns of readability on a variety of displays, including terminals and mobile displays, when they exist solely for reading comprehension, are easy to wrap, and rarely have cases where you really need an especially long line, as code often does.

Hopefully this makes my motivation clear without too much rambling; sorry about the crazy length.

@matthew-mcallister
Copy link
Contributor Author

matthew-mcallister commented Oct 26, 2017

P.S. I do agree that it is wise to conserve the number of available options, but I would suggest combining or pruning existing options and not rejecting this one if that is really such a big concern. For example, there are around half a dozen options just for specifying when to put a space before/after a colon, and there is an option to put spaces inside of non-empty parens, which is not the Rust convention and also fairly arbitrary.

@mgeisler
Copy link

In fact, at first I thought rustfmt's comment wrapping was bugged; I've never even heard of anyone following the convention set by the default behavior.

@matthew-mcallister are you saying that setting comment_width = 72 means "allow comments to be up to 72 columns wide, disregarding any leading indentation"? That does sound quite unexpected to me.

@matthew-mcallister
Copy link
Contributor Author

Yep, I noticed the behavior right away. It doesn't seem like it was unintended as far as I can tell.

Also, I just rebased this on master and pushed a fix for a corner case (which may be squashed before merging).

@nrc
Copy link
Member

nrc commented Oct 31, 2017

As an example of why comment_width is the way it is, consider a situation where the max width is 120 and the comment width is 60, for a line that has already been indented 40 spaces, you get to make a readable block of comments 60 columns wide, rather than a pointlessly narrow comment 20 columns wide. That is an extreme case, but the logic holds for less extreme examples.

@nrc
Copy link
Member

nrc commented Oct 31, 2017

Hopefully this makes my motivation clear without too much rambling; sorry about the crazy length

Sorry to be argumentative but neither of the reasons you give here motivates adding absolute_comment_width:

1 is addressed by having comment_width - this enables wrapping comments at a narrower width. 2 is not an issue because comments are still limited by max_width, so a comment can never be wider than a code line and thus will never be the reason for requiring a wider window.

@matthew-mcallister
Copy link
Contributor Author

matthew-mcallister commented Oct 31, 2017

@nrc I sense there's a big misunderstanding here. The purpose of the change is to enable comments to have shorter lines than code so that they fit into narrow terminals/windows. Indeed, the current behavior will keep comments from being subjectively too wide for readability, like newspaper columns do. However, it does not address the latter issue; it actually makes the issue worse.

Let me address the preceding comments directly and then give a proposal.

consider a situation where the max width is 120 and the comment width is 60, for a line that has already been indented 40 spaces

You can already observe this extreme case with the default behavior by indenting your comment to 100 characters; doing so would be an egregious style mistake, though, which rustfmt will then make obvious for you.

comments are still limited by max_width, so a comment can never be wider than a code line and thus will never be the reason for requiring a wider window.

Yes, they can cause the need to resize windows, and the default rustfmt settings make it a very common occurrence. Consider an 8-space indented comment on an 80-column display; the comment then extends to column 88, and the resulting double wrapping looks atrocious. Here's a sample rendering:

impl Foo {
    fn bar() {
        // Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusm
od
        // tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
veniam,
        // quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commo
do
        // consequat. Duis aute irure dolor in reprehenderit in voluptate velit
esse
        // cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupid
atat non
        // proident, sunt in culpa qui officia deserunt mollit anim id est labor
um.
    }
}

Sure, one possible workaround is to shrink max_width to 80. But, when it comes to code, there are obvious advantages to allowing a line width of, say, 100. There shouldn't have to be a choice between excessive linewrapping in comment blocks and excessive reformatting of code to get it to fit into the smallest common denominator of screen widths.

With that explained, how about I instead change this PR to leave comment_width alone and add a new integer setting, comment_line_width, which sets the absolute wrapping width? That would make it clear that the new option is specifically for addressing line wrapping problems on vertical displays, enabling better formatting in a realistic variety of environments.

@nrc
Copy link
Member

nrc commented Nov 1, 2017

I understand your motivation, but I'm not convinced of the trade-off. For one thing, long lines of code are not uncommon in Rust, so I think that in whatever system deals with comments, the long code will make side-by-side windows inconvenient unless you choose a suitable max_width. Second, in practice the big blocks of comments are nearly always at low numbers of indent (on modules or other top-level items), so it would be rare for the two comment restrictions to have much practical difference.

On the other hand, I think the negatives are somewhat large. There was some pushback about even having a separate width for comments, and I think having three ways to configure the width would be confusing.

So, one could argue that the proposed measure of comment width is better than the current one. My counter-example is the common situation where you have a 100/80 setup (defaults) and blocks of comments on zero-indent and four-indent items (e.g., on the module and on each item in the module). In either case, the width of the window is constrained by the code width, not the comment width. But with the current setup you get to have all comment blocks be the 'comfortable comment' width, rather than only the zero-indented one, and the others a bit narrower.

I don't think that is a large advantage, nor an objective one. So we might revisit the decision. We have agreed on this in the formatting RFC process, but there will be another chance to discuss when we post a write-up as an ordinary RFC.

@mgeisler
Copy link

mgeisler commented Nov 1, 2017

consider a situation where the max width is 120 and the comment width is 60, for a line that has already been indented 40 spaces, you get to make a readable block of comments 60 columns wide.

@nrc I'm not 100% sure what this means :-) Are you saying that the comments are allowed to grow until the total line length hits the max width configured?

The whole point of specifying a maximum width for the comment lines is to avoid having them stick out in the margin. That means that if I ask for comment lines to be a maximum of 60 chars, then they entire line must not be longer than this. If that length is only respected sometimes (depending on indentation), well then the length becomes meaningless since I need to resize my window depending on the code at hand.

I just read your comments more carefully and found this:

comments are still limited by max_width, so a comment can never be wider than a code line and thus will never be the reason for requiring a wider window.

That makes sense! Thanks :-)

@matthew-mcallister
Copy link
Contributor Author

@nrc

So we might revisit the decision. We have agreed on this in the formatting RFC process, but there will be another chance to discuss when we post a write-up as an ordinary RFC.

OK, an RFC for this sounds reasonable to my ears, if that's what you mean. I'm not familiar with the RFC process here, though; can you point me to more relevant information, i.e. timeframe, decision-making, and actionable items on my part? I assume such a proposal would clear up secondary issues such as configuration/documentation and would include at least 3 possible outcomes: make this behavior optional; make this behavior the default; do nothing.

I'm definitely convinced that I've somehow failed at explaining here; it sounds like you want to throw the baby out with the bathwater. Maybe it takes a certain perspective for it to be obvious? In any case, it seems like we can both agree that input from others is needed. I'm open ears for recommendations of what to do next.

@nrc
Copy link
Member

nrc commented Nov 2, 2017

The process for deciding formatting has mostly completed over on https://github.com/rust-lang-nursery/fmt-rfcs We're currently writing up a style guide. That (the style guide) will be posted as an RFC for final discussion and sign-off on the RFCs repo (https://github.com/rust-lang/rfcs) probably around the end of the year.

@matthew-mcallister
Copy link
Contributor Author

Thanks for pointing me to the RFCs. Based on the discussion I discovered at rust-lang/style-team#17, it seems like the consensus on best practices is to do wrapping of comments by hand. I find the arguments there totally convincing, and disabling automatic wrapping frees me to set any convention on my own code (at the cost of manual enforcement), so this PR no longer has a purpose as far as I'm concerned.

Make no mistake: this convention solves many concrete shortcomings of the default convention. However, I'm obviously not interested in changing the status quo for projects I don't maintain, and trying to do so would be a fool's game anyways, so this is basically an ideal outcome as far as I'm concerned.

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

3 participants