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

Decide on guiding principles #4

Closed
nrc opened this Issue Aug 23, 2016 · 37 comments

Comments

Projects
None yet
@nrc
Collaborator

nrc commented Aug 23, 2016

We should decide on the principles behind the formatting guidelines and their relative priority.

Some ideas (not in order):

  • internal consistency
  • readability (needs further expansion)
  • scan-ability (ditto)
  • preventing right-ward drift
  • preserving diffs
  • minimising vertical space
  • helpfulness to tools?
  • consistency with formatting used in other languages (either automatic, or defined in style guides)
  • ease of manual formatting
  • ease of automatic formatting for editors not using Rustfmt
  • ease of implementation
  • ease of re-implementation in other formatting tools
  • simplicity of formatting rules
  • aesthetics (boy, does this need further expansion)
@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Aug 25, 2016

  • ease of searching through code
  • avoiding misleading formatting (this may get merged with "readability" or similar, but I think we should explicitly state it)
  • compatibility with version control practices (does "preserving diffs" above mean the same thing?)
  • readability of code when quoted in rustc error messages
  • simplicity for code generators outputting Rust code
  • consistency between code format and documentation format

On that last point: a fmt RFC may sometimes require an analogous change to rustdoc to keep formatting consistent.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Aug 25, 2016

Another related item:

  • To what extent should rustfmt make non-whitespace changes to code as part of formatting? (Parentheses, commas, semicolons, changes to string constants, etc)
@nikomatsakis

This comment has been minimized.

nikomatsakis commented Sep 3, 2016

It seems like settling on how to weigh the "ease of manual formatting" is a key question that will affect a lot of things. I know that my own style has been heavily influenced by the fact that emacs basically does it all for me automatically (and, in general, if emacs doesn't do what I like, then I decide to like what emacs does, or else tweak things a bit to get it to do something I like more).

When for a brief period I lost my mind switch to TextMate, which had very primitive formatting capability, I adopted a much simpler style that basically involved "newline and then indent one tab" rather than "align something with the previous line" (which I think is sometimes called "visual indent").

I guess that even though I mostly prefer visual indent, I am forced to admit that ease of manual formatting may be better overall, since there are so many editors out there which people like to use, and most of them don't have sophisticated support (plus we sometimes have to edit code in annoying places). That said, since we have rustfmt, it can be the "tool to rule them all" to some extent and correct for the shortcomings of most editors.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 3, 2016

@nikomatsakis If rustfmt produces output that everyone can accept with approximately zero exceptions, and people format their code with rustfmt as standard practice, then that will address any issues of formatting inconsistency.

That said, I do think that the preferred style should come naturally, and should not prove difficult to manually format while entering code. It's reasonable to expect people writing a non-trivial amount of code to have editor support, but it's not reasonable to expect people to follow a style that nobody would come up with on their own without reference to the style guide. The job of the style guide is to make the call on various individual formatting bikeshed decisions that could easily go either way, not to define an elaborate non-trivial style that nobody would have guessed naturally.

That raises one key question about rustfmt, though: do we expect that rustfmt will always ignore all existing whitespace (including newlines) and re-break all lines as it sees fit, or do we expect that it will take existing newlines and alignment into account? If the former, rustfmt will break code that has carefully inserted line-breaks and visual alignment. If the latter, then rustfmt can produce different results for the same series of tokens depending on its existing indentation. Neither one of those seems like an entirely satisfying result.

@nrc

This comment has been minimized.

Collaborator

nrc commented Sep 3, 2016

That raises one key question about rustfmt, though: do we expect that rustfmt will always ignore all existing whitespace (including newlines) and re-break all lines as it sees fit, or do we expect that it will take existing newlines and alignment into account?

We experimented quite a bit with this, my conclusion was that Rustfmt should basically ignore any existing formatting - it is simply too difficult to integrate existing formatting with new formatting, so either we do no formatting or all formatting.

Two caveats: we should avoid formatting comments and string literals too much because of this problem. It would be nice to word wrap comments, but doing this without ruining existing formatting proved very hard. There should be an opt-out - e.g., #[rustfmt(skip)], which allows you to do your own formatting without Rustfmt touching it when necessary.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 3, 2016

Two caveats: we should avoid formatting comments and string literals too much because of this problem.

Agreed.

We might be able to handle comments, if we assumed that alignment of items within comments would only occur within a Markdown code block or otherwise managed by Markdown formatting (such as a list). I don't know that we can assume that, though.

There should be an opt-out - e.g., #[rustfmt(skip)], which allows you to do your own formatting without Rustfmt touching it when necessary.

That seems reasonable, assuming we can scope it to apply as narrowly as necessary. (And we should take any occurrence of it as a challenge: "could rustfmt be smarter?".)

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 4, 2016

One more guiding principle:

  • Similarity to existing languages. Take advantage of people's existing experience with other programming languages, and avoid gratuitous style differences for analogous constructs.
@bruno-medeiros

This comment has been minimized.

bruno-medeiros commented Sep 5, 2016

@nrc

We experimented quite a bit with this, my conclusion was that Rustfmt should basically ignore any existing formatting - it is simply too difficult to integrate existing formatting with new formatting, so either we do no formatting or all formatting.

I disagree, I think there should at least be an option for this, for considering existing newlines. Imagine for example you have a function like this:

fn xpto(
    name_one: String, name_two: String,
    some_path_parameter: PathType,
    option_one: bool, option_two: bool,
    some_other_parameter: SomeOtherType,
) {
    //...
}

And you want to keep name_one and name_two on the same line, because they are somehow related semantically, and same thing with option_one and option_two: you want to keep them together.

Formatting and ignoring existing newlines is likely to result in a more disjointed grouping of parameters:

fn xpto(
    name_one: String, name_two: String, some_path_parameter: PathType, option_one: bool, 
    option_two: bool, some_other_parameter: SomeOtherType,
) {
    //...
}

Assuming the tool is not using ubsan's style that is. But even formatting under ubsan's style (one parameter per line) might not as good as the original code.

@solson

This comment has been minimized.

Contributor

solson commented Sep 5, 2016

@bruno-medeiros If rustfmt can't even rearrange arguments, that really limits its usefulness as a tool. Personally I don't think it's worth having an arbitrary escape hatch in the style rules for when a programmer feels things should be grouped in a non-standard way. It's more predictable if it's always standard.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 5, 2016

At first, I wanted to suggest counterexamples, where grouping arguments makes sense: coordinates of a point, dimensions of a rectangle, pointer and length of an array...until I realized that every one of those examples either wants a tuple or a dedicated data type. I can't actually think of a single good example where it makes sense to group options without creating an appropriate data type or using a tuple.

@nrc

This comment has been minimized.

Collaborator

nrc commented Sep 15, 2016

From #2, @nikomatsakis:

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".

@briansmith

This comment has been minimized.

briansmith commented Sep 16, 2016

I would like you to consider accessibility as a key principle.

Specifically, I am not even considered "low vision" by any standard but in order to sit at my laptop all day long I have to have my font size set such that GitHub's side-by-side diffs barely fit two 80-column panes next to each other and such that my 3-way-merge tool doesn't even get close to showing 80 columns of all three panes. I've tried to make 100-character columns work in my projects since that is what people seem to prefer, but I physically cannot do it in a reasonably way, which is why I use an atypical-for-Rust 80-character max in my projects.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 16, 2016

@briansmith First, I'd like to completely agree with your suggestion of accessibility as a key principle. Rust's coding standards should not cater primarily to people with great vision, small fonts, or large/multiple monitors.

FWIW, I don't think the majority of users have fonts or terminals that allow three 80-column windows next to each other, and many don't even have setups that allow two 80-column windows. Some users might, but most don't. I have a 2560x1440 monitor, and I still only have 196 columns in my terminal; if I use the next font size up, I have 159 columns. Many users will code on 1920x1080 or 1366x768 monitors. I don't think a three-way merge with three vertical columns is a reasonable argument for either approach, as only people with huge high-resolution monitors or tiny fonts will be able to fit 242 columns, let alone 302 columns.

Personally, I do a three-way merge with old and new side-by-side above the working/editing window. I also don't tend to find it problematic when a line exceeds the width of the window; I find my editor's presentation of such a line reasonable.

I also consider smaller screens a good argument for 4-space indents: I find that the wider indent makes it much easier to keep track of indentation levels when paging through a block of code that you can't see all at once.

@ubsan

This comment has been minimized.

Contributor

ubsan commented Sep 16, 2016

@briansmith I'd argue that 80-characters is not atypical for Rust, it's just not standard.

@briansmith

This comment has been minimized.

briansmith commented Sep 16, 2016

preserving diffs

I think this is an unnecessary goal that has harmful consequences. A good diff tool can highlight the diff in the "right" way that allows one to see differences caused by (re)formatting vs. other differences. Google's gerrit code review tool seems to do this, for example, and so does SourceGear DiffMerge. GitHub and other tools can be improved in this area; it's not worth optimizing for their current deficiencies at the cost of other factors, in particular the other principle of avoiding wasting vertical space.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 16, 2016

@briansmith I'd like to say "git diff" made smarter, sure. But as long as it isn't, it's worth at least considering what it doesn't handle. That doesn't mean that consideration will always win, just that it shouldn't be dismissed out of hand as a useful criteria.

@briansmith

This comment has been minimized.

briansmith commented Sep 16, 2016

@briansmith I'd like to say "git diff" made smarter, sure. But as long as it isn't, it's worth at least considering what it doesn't handle. That doesn't mean that consideration will always win, just that it shouldn't be dismissed out of hand as a useful criteria.

Keep in mind that the limitations of git diff and friends are much more malleable than a formatting standard would be. In particular, let's say that in 2019 git diff is made as good as gerrit or DiffMerge. Would we then rewrite the formatting guide and have people reformat to the new standard? I think we'd avoid that. I think instead we should encourage people to improve git diff by not optimizing (at all) for its deficiencies, when other tools already fixed them.

@gkoz

This comment has been minimized.

gkoz commented Sep 18, 2016

preserving diffs

This also affects merge conflicts. @briansmith are the existing good merging tools capable of ignoring whitespace changes so e.g. a single-line conflict does not get amplified due to formatting differences?

In particular, let's say that in 2019 git diff is made as good as gerrit or DiffMerge. Would we then rewrite the formatting guide and have people reformat to the new standard?

At a point when a bigger number of tools are good and make that kind of reformatting transparent, maybe?

Along the lines of other considerations

ease of manual formatting
ease of automatic formatting for editors not using Rustfmt

I'd find it nice if the style did not leave users with less advanced/popular/perfectly configured tools in the cold.

@briansmith

This comment has been minimized.

briansmith commented Sep 18, 2016

This also affects merge conflicts. @briansmith are the existing good merging tools capable of ignoring whitespace changes so e.g. a single-line conflict does not get amplified due to formatting differences?

That's a good question and I don't know the answer. I know that SourceGear DiffMerge can automatically resolve more merges than git rebase does internally, but I don't know if the reason is due to smarter handling of re-wrapping.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 18, 2016

Changing more unrelated lines of code does tend to break merges, rebases, patch application, and various other tools. Some cleverer tools might manage to avoid that, but I still think "don't cause rippling changes outward from a single modification" seems like a reasonable consideration to keep in mind.

@nrc

This comment has been minimized.

Collaborator

nrc commented Sep 20, 2016

I have attempted to condense the list of possible principles from throughout this thread, ordering is my personal opinion on prioritisation:

  • readability
    • scan-ability
    • avoiding misleading formatting
    • accessibility - readable on the widest variety of hardware
    • readability of code when quoted in rustc error messages
  • aesthetics
    • sense of 'beauty'
    • consistent with other languages/tools
  • specifics
    • compatibility with version control practices - preserving diffs, merge-friendliness, etc.
    • preventing right-ward drift
    • minimising vertical space
  • application
    • ease of manual application
    • ease of implementation (in Rustfmt, and in other tools/editors/code generators)
    • internal consistency
    • simplicity of formatting rules

And there was one which I didn't really understand:

consistency between code format and documentation format

How do people feel about these principles?

@nrc

This comment has been minimized.

Collaborator

nrc commented Sep 20, 2016

Some commentary on the above list:

I hope it is obvious why the readability issues are most important. In particular I feel scannablility of code is important - I rapidly get frustrated with a style if it prevents me quickly navigating around code without reading too deeply.

I think aesthetics are important because I spend a lot of time looking at code and I don't want to be repulsed by ugliness when I do so. I think it is easy to shy away from this because it is difficult to be objective, but just because something is subjective does not mean it should be ignored. We should, however, be aware of out prejudices, in particular, familiarity.

We have strived hard in designing the syntax of Rust to be familiar to as many programmers as possible, without sacrificing functionality. I believe we should do so here too. It is good for Rust as a whole if many programmers can come to Rust code and feel familiar with the style.

I realise VCS interaction is important to many people and I believe we should take it into account. But primarily, code is read by humans, not tools, and we spend a lot more time reading code than merging it or exploring VCS repos, therefore I think we should not sacrifice either readability or aesthetics for VCS compatibility.

Likewise, we should minimise use of horizontal and vertical space, but not at the expense of readability. In particular, I find that styles which are quite 'spacey' vertically are much easier to read and scan than dense code. I feel it is important for the code to have a shape.

Finally, I think that ease of implementation should not be taken too much into account, with the caveat that it must be possible to implement.

Ideally it is easy to apply the style manually, but I find this very hard to quantify - what is manual? Every editor has different tools here, so it is hard to judge. Furthermore, if using Rustfmt at all, you can just type any old format and then apply Rustfmt.

Internal consistency should be a goal where possible, but I'm happy to override wherever necessary to provide a better user experience.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 20, 2016

"consistency between code format and documentation format" referred to ensuring that rustdoc documentation matches the corresponding code.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 20, 2016

I agree completely with the above principles, and almost completely with the ranking. I'd put "simplicity of formatting rules" at the top of "application". Apart from that, 👍

@nrc

This comment has been minimized.

Collaborator

nrc commented Sep 20, 2016

as in the code generated by Rustdoc?

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 20, 2016

Right, as well as code included in doc comments that gets propagated into rustdoc.

@keeperofdakeys

This comment has been minimized.

keeperofdakeys commented Sep 20, 2016

One thing I don't think you've touched on is editability. Code should be easy to modify, while retaining the current style. A style that involves lots of precise alignment, or indenting, may be hard to modify in a simple editor (for example, a quick GitHub edit). Running rustfmt at the end to fix up small style issues is fine (comment wrapping, small indentation mistakes), but you shouldn't need to continuously run it if the style is too hard to manually write with.

@nrc

This comment has been minimized.

Collaborator

nrc commented Sep 20, 2016

@keeperofdakeys this is part of what I meant by "ease of manual application"

@briansmith

This comment has been minimized.

briansmith commented Sep 20, 2016

One thing I don't think you've touched on is editability. Code should be easy to modify, while retaining the current style.

At least in Go land, and with hindent for Haskell, you just do cargo fmt && git add ... && git commit and you don't have to worry about the formatting while you edit the code. Thus, I don't think this needs to be considered a major concern.

@briansmith

This comment has been minimized.

briansmith commented Sep 20, 2016

Likewise, we should minimise use of horizontal and vertical space, but not at the expense of readability. In particular, I find that styles which are quite 'spacey' vertically are much easier to read and scan than dense code.

This is a micro vs. macro issue. A specific line of code might be easier to read if it is split across N lines. But, a function is also easier to read if one can read it without scrolling, and a set of related functions is easier to read if they all fit on the screen together. The idea of minimizing usage of vertical space is to optimize for these macro readability concerns, at the cost of the micro concerns.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Sep 26, 2016

@nrc Based on feedback from @camlorn in #8, I'd like to propose a modification to the "accessibility" point: it's not just "readable on the widest variety of hardware", but also "readable and editable by users using non-visual accessibility interfaces".

@camlorn

This comment has been minimized.

camlorn commented Sep 26, 2016

@joshtriplett @nrc
I'd appreciate it, yes, though the thing about completely nonvisual interfaces (which I also mention in #8) is that it's almost entirely nonstandard.

Humans just aren't built to think about the world without vision, so everyone comes up with their own coping strategies. Python is a perfect concrete example of this. I use Python fine and like it, but there are many new blind programmers who claim that Python is inaccessible to blind people because of the indent thing, and I've got at least 2 friends who don't set the screen reader to indicate indentation while coding Python because they can just infer where and what it should be (me and another 2 use indent with everything because we can't and find it helpful to match braces). Then the tabs vs. spaces argument takes on a whole new life, with some of us siding with spaces because the sighted people use it and some of us siding with tabs because that makes the indent level exactly equal to the level of the block in terms of (metaphorical or otherwise) braces.

And then you get into stuff like the doc comments which I hate but can live with, etc.

CC me on discussions where it comes up and I'll monitor them,. Not too sure we have many other blind programmers around here, and I wish we did for this. One person isn't consensus.

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 3, 2016

I'd put "simplicity of formatting rules" at the top of "application"

The reason I put it at the bottom, is that for me, concrete factors that affect users directly (e.g., ease of manually applying) are more important than abstract factors that affect users indirectly (why is simplicity important? For me, because it enables ease of application and implementation. Simplicity for its own sake (i.e., not covered by the higher priority points) is relatively low priority.

I'd like to propose a modification to the "accessibility" point: it's not just "readable on the widest variety of hardware", but also "readable and editable by users using non-visual accessibility interfaces".

SGTM, modulo @camlorn's comment

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 3, 2016

I believe the guidelines in #4 (comment), modified with #4 (comment) are ready to go. I'm thus labelling this issue as being in FCP to give a last chance to comment on our guiding principles.

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 3, 2016

  • last chance before we make a PR, which will be another chance, but not a very long one.
@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 3, 2016

@briansmith

This is a micro vs. macro issue. A specific line of code might be easier to read if it is split across N lines. But, a function is also easier to read if one can read it without scrolling, and a set of related functions is easier to read if they all fit on the screen together. The idea of minimizing usage of vertical space is to optimize for these macro readability concerns, at the cost of the micro concerns.

I agree there is a trade-off here and I do believe in trying to minimise vertical space for this reason. However, I still think it is lower priority since I think speed of scanning is more important than speed of detailed reading (the latter is always slow) and for scanning, once you start scrolling, I think the amount you must scroll is less important.

I also think that as time passes and larger, higher density displays are more common, plus tools that make side-by-side viewing easier, minimising vertical space is getting less important.

nrc added a commit that referenced this issue Oct 12, 2016

@nrc nrc removed the ready-for-PR label Oct 12, 2016

@Screwtapello

This comment has been minimized.

Screwtapello commented Oct 14, 2016

I can't actually think of a single good example where it makes sense to group options without creating an appropriate data type or using a tuple.

The only time this has happened to me is when I'm building a command-line, because those frequently intermix key/value and positional arguments:

[
    "/path/to/some/tool",
    "--opt1", "something",
    "--opt2", "another thing",
    "arg1",
    "arg2",
]

But it's a pretty niche use-case, especially for Rust, and the benefits of "just use rustfmt" would outweigh the readability improvements.

@nrc nrc closed this in #28 Oct 16, 2016

@gbutler69 gbutler69 referenced this issue Jul 6, 2017

Closed

Define 'short' #47

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