Format all Markdown files with dprint

[mgeisler]

This is the result of running dprint fmt after removing src/ from the list of excluded directories.

This also reformats the Rust code: we might want to tweak this a bit in the future since some of the changes removes the hand-formatting. Of course, this formatting can be seen as a mis-feature, so maybe this is good overall.

Thanks to mdbook-i18n-helpers 0.2, the POT file is nearly unchanged after this, meaning that all existing translations remain valid! A few messages were changed because of stray whitespace characters:

 msgid ""
 "Slices always borrow from another object. In this example, `a` has to remain "
-"'alive' (in scope) for at least as long as our slice. "
+"'alive' (in scope) for at least as long as our slice."
 msgstr ""

The formatting is enforced in CI and we will have to see how annoying this is in practice for the many contributors. If it becomes annoying, we should look into fixing dprint/check#11 so that dprint can annotate the lines that need fixing directly, then I think we can consider more strict formatting checks.

I added more customization to rustfmt.toml. This is to better emulate the dense style used in the course:

• max_width = 85 allows lines to take up the full width available in our code blocks (when taking margins and the line numbers into account).
• wrap_comments = true ensures that we don't show very long comments in the code examples. I edited some comments to shorten them and avoid unnecessary line breaks — please trim other unnecessarily long comments when you see them! Remember we're writing code for slides 😄
• use_small_heuristics = "Max" allows for things like struct literals and if-statements to take up the full line width configured above.

The formatting settings apply to all our Rust code right now — I think we could improve this with dprint/dprint#711 which lets us add per-directory dprint configuration files. However, the inherit: true setting is not yet implemented (as far as I can tell), so a nested configuration file will have to copy most or all of the top-level file.

[mgeisler]

Oh, I should have mentioned: the formatting of dprint seems to follow the Google developer documentation style guide:

• strong emphasis uses **, not __
• emphasis uses _, not *

I don't know if we recommend using - instead of *, but I've grown to like the new lists: their formatting seems lighter and I like that.

[qwandor]

How many columns is this wrapping at? Could we configure it for 100?

[djmitche]

I would argue we should not do this right now -- this is going to cause a bunch of trivial but hard-to-resolve conflicts with the v2 branch.

When we do make this change, I would strongly urge that we run it in CI. I can almost guarantee that every PR I make will use * instead of -, or wrapped differently, or something else -- triggering a spurious change within the next week and polluting the change history of the repo. Maybe it's worth waiting until dprint/check#11 is done so that this is easier to use via edits in GitHub?

[djmitche]

(removing review request; see comments above)

[mgeisler]

How many columns is this wrapping at? Could we configure it for 100?

Personally, I don't get why people want long ragged lines since I find those harder to read. So I will want to keep this at 80 columns (which is what the course was originally wrapped at when I first open sourced it). We also wrap our Rust code at 80 columns in the course so it can fit on the slides when projected.

I would argue we should not do this right now -- this is going to cause a bunch of trivial but hard-to-resolve conflicts with the v2 branch.

Oh, I had no considered this! I'll put it on pause for now.

When we do make this change, I would strongly urge that we run it in CI. I can almost guarantee that every PR I make will use * instead of -, or wrapped differently, or something else -- triggering a spurious change within the next week and polluting the change history of the repo.

Maybe it's worth waiting until dprint/check#11 is done so that this is easier to use via edits in GitHub?

I would not want to wait on that issue, as you can see it's been open for a while 🙂 But you raise a good point: let me see if I can find someone who will help us work on it.

[djmitche]

Personally, I don't get why people want long ragged lines since I find those harder to read.

I don't mind either way, but the rationale for sentence-per-line in markdown is that it makes diffs more sensible. Wrapping a paragraph means that any change at the beginning of the paragraph which causes a re-wrap will "change" the entire paragraph, leading to spurious conflicts, bad git blame information, and slightly more difficulty in review. This last point used to be worse, but GitHub and other review tools are pretty good at "reversing" the wrapping these days and highlighting only the changed words.

[mgeisler]

Personally, I don't get why people want long ragged lines since I find those harder to read.

I don't mind either way, but the rationale for sentence-per-line in markdown is that it makes diffs more sensible. Wrapping a paragraph means that any change at the beginning of the paragraph which causes a re-wrap will "change" the entire paragraph, leading to spurious conflicts, bad git blame information, and slightly more difficulty in review.

To be very precisely and pedantic, hard-wrapping the paragraph can lead to fewer merge conflicts: you can now fix a typo in line 1 while I remove a word in line 10 🙂 That would not be possible if the paragraphs are completely unfolded (which I don't think Andrew had in mind).

Diffs of long lines will have to break at some point in the various UI. They won't look very nice in a terminal or a text based editor line my Emacs. In GitHub, they also don't look brilliant since they necessarily have to show the entire paragraph in the - and + lines, even if you only changed part of it.

This last point used to be worse, but GitHub and other review tools are pretty good at "reversing" the wrapping these days and highlighting only the changed words.

Yeah, that highlighting is super useful! Very tangential, I advocated for adding such highlighting to the googletest library a while ago: google/googletest-rust#221. I hope we'll get it at some point since it makes life easier for all users 🙂

[djmitche]

Sorry, the line-wrapping mode I was describing is a line-break at each sentence break.
Something like this.
It works well because words do not typically migrate from one sentence to the next.
It has a beneficial side effect, as well: shorter sentences are generally preferred for readability, and this format makes sentence length visible.

Anyway, I was just providing data to the "I don't get why ..". I'm happy to wrap content in this course, as its paragraphs tend to be short and edits tend to be made at the paragraph or slide level, not the sentence level.

[mgeisler]

Sorry, the line-wrapping mode I was describing is a line-break at each sentence break.

Oh, sorry... I misread your comment!

[randomPoison]

Personally I'm not a fan of manually enforcing formatting. Realistically it's something we're going to forget to do on a regular basis, and then when do run a formatting pass we're going to end up with a large set of changes that will cause conflicts for any pending PRs. I get the desire to make contributions as quick and easy as possible, but I think if we're worrying about formatting we should be enforcing it in CI.

That said, @mgeisler do we even want to keep this PR open? If we do run a formatting pass after v2 merges it'll have to be redone anyway, right? So may as well close this PR and open a new one when ready.

[mgeisler]

Personally I'm not a fan of manually enforcing formatting. Realistically it's something we're going to forget to do on a regular basis

Do you mean that you also prefer enforcing this with a GitHub action like @djmitche said?

I have been holding off on adding too many such checks to make it super easy for people to contribute, but now that we have such a check for the PO files (#1359), my impression is that it actually works well: people get the error, run dprint fmt, and then continue their work.

It does cause problems for drive-by commits where people edit directly in the GitHub editor... for those, I suggest that maintainers like you and I use something like

gh pr checkout NNN; dprint fmt; git commit --all --amend

to fix any formatting issues in-flight after reviewing the PR. I don't know how much more work this will be, though...

when do run a formatting pass we're going to end up with a large set of changes that will cause conflicts for any pending PRs

There should be no conflicts when you format the branch you want to merge in. In particular if you squash your branch into a single commit (which you might as well do since we do that with every merge here).

A 3-way merges normally compare the final state on each branch. To test this, I made a little repository with two conflicting commits which ultimately ended up with the same final state:

% git init merge-test
% cd merge-test
% echo start > a.txt
% git commit -m 'start'
% git checkout -b foo
% echo foo > a.txt
% git commit -m 'foo'
% git commit -m 'final on foo'
% git checkout master
% git checkout -b bar
% echo bar > a.txt
% git commit -m 'bar'
% echo final > a.txt
% git add a.txt
% git commit -m 'final on bar'
% git merge foo

The merge was clean and the final history looks like this:

b58000b *   bar Merge branch 'foo' into bar
        |\  
799fbd7 | * foo final on foo
e094585 | * foo
0547010 * | final on bar
4486e86 * | bar
        |/  

This is different from a rebase, which would effectively apply the commits of the branch one-by-one.

So I hope this property makes it easy to work with the PRs.

That said, @mgeisler do we even want to keep this PR open? If we do run a formatting pass after v2 merges it'll have to be redone anyway, right? So may as well close this PR and open a new one when ready.

I was simply planning on rebasing the PR on top of main when we have merged the v2 work. So the PR is a bit of a reminder to myself to do this and a reminder that others don't need to work on this 😄

[randomPoison]

Do you mean that you also prefer enforcing this with a GitHub action like @djmitche said?

Yup, exactly.

It does cause problems for drive-by commits where people edit directly in the GitHub editor... for those, I suggest that maintainers like you and I use something like

gh pr checkout NNN; dprint fmt; git commit --all --amend

This might work for PRs that are coming from a branch within the repo, but won't work for forks because you can't push commits to someone else's repo.

[djmitche]

I think it's possible for a GH action to do that kind of update?

Also, I believe GH's default is to allow pushes to forks, so at least where authors haven't disabled that for their PR, it would be possible to push to the fork.

[mgeisler]

This might work for PRs that are coming from a branch within the repo, but won't work for forks because you can't push commits to someone else's repo.

Also, I believe GH's default is to allow pushes to forks, so at least where authors haven't disabled that for their PR, it would be possible to push to the fork.

Yes, it is an option the author picks when creating the PR, by default admins can indeed push to the branch used for a PR, even if it's in another repo. It's a bit surprising at first, but it totally works — including force-pushes 😄

[mgeisler]

Do you mean that you also prefer enforcing this with a GitHub action like @djmitche said?

Yup, exactly.

Great, then we're on the same page 😄 Ideally, we should solve dprint/check#11 which (I think) would make it possible for the action to emit proper PR suggestions.

But we'll take that as it comes.

[djmitche]

I'm conceptually in favor of this, although I'm not going to review the diff :)

[mgeisler]

Thanks @djmitche for the review, I rebased the branch (threw away the formatting but kept the config file changes) and updated it a little to avoid lines overflowing the horizontal space on the slides.

I'm curious to see how this impacts the PRs going forward.