Amphtml: Shall we auto-format our `.md` files with Prettier?

[rsimha]

Today, we use Prettier to format and fix all our JS files and several non-JS files. The format checker has supported Markdown files for a while now.

Question: Shall we start auto-formatting our .md files with Prettier?

Pros:

• One single standard for the internal structure of documentation files
• IDE support for auto-fix-on-save while editing code
• Command line support for auto-fixing local changes
• Possible to override Prettier's auto-format behavior for sections of code
• Possible to auto-wrap text for easy editing

Cons:

• There might be some small changes to how .md files look when they are rendered
  
  
  
  • Attribute lists are changed to all-in-one-line if they all fit on one line
    
    
  
  
  • Attribute lists are changed to one-per-line if they don't all fit on one line
    
    
  
  

• Failing gulp prettify checks can be annoying if an occasional contributor makes a badly formatted change
• No easy way to auto-format code with Prettier if you edit code with the GitHub web-UI

@CrystalOnScript @mrjoro @pbakaus @ampproject/wg-outreach Thoughts?

[rsimha]

I took a stab at auto-fixing all our .md files by running gulp prettify on them. Here are the results. The only visible changes to rendered pages are in code samples, where attributes are now one per line. (It might be possible to disable this behavior.)

[mrjoro]

I frequently make doc updates using the web UI. Is there any way to hook this up for PRs?

Also, looking at the samples it appears prettify incorrectly removes one of the two spaces after a period.

[rsimha]

I frequently make doc updates using the web UI. Is there any way to hook this up for PRs?

Did you mean hook up an auto-corrector to the GitHub web-UI? (Not sure if such a thing exists.)

For regular non-web-UI usage, there is a command line auto-fixer.

gulp prettify --local_changes --fix

Also, looking at the samples it appears prettify incorrectly removes one of the two spaces after a period.

Yeah, I figured someone would complain about this :) The parser used under the covers is remark-parse. I'd have to check if it can be configured to use double-spacing. (It's worth noting that this doesn't affect how the .md files are rendered in HTML.)

[CrystalOnScript]

I love the idea of having a checker for the .md files to prevent small mistakes coughmecough, but I'm unsure if it would effect how they're rendered in amp.dev. CC @sebastianbenz

[rsimha]

Most of prettier's auto-formatting changes to our .md files will not affect how they're rendered on amp.dev. The only exception I noticed is that AMPHTML attributes are re-formatted to be one on each line. See screenshot below (red = old, green = new).

And then, there's the fact that two-spaces-after-period are changed to one-space-after-period. It doesn't affect how the markdown looks when it's rendered as HTML, but it can become annoying for those who are used to typing two spaces. I think there's some value in standardizing how our docs are written in raw markdown. (I tried, and couldn't find a way to override this behavior.) @mrjoro can comment on whether or not this a deal-breaker.

[rsimha]

Tagging a few working groups for comment.

@ampproject/wg-runtime
@ampproject/wg-performance
@ampproject/wg-ads
@ampproject/wg-analytics
@ampproject/wg-stories
@ampproject/wg-access-subscriptions
@ampproject/wg-ui-and-a11y
@ampproject/wg-infra

[mrjoro]

My comment about two spaces after the period was mostly in jest; I'm okay with us standardizing on the incorrect single space after a period. ;)

I don't suppose one of the PR checks we run could automatically commit a prettified form if the PR was generated using the web UI? My concern would be that I make a non-pretty change in the web UI, then the next person editing the doc prettifies the doc and gets a lot of unrelated changes in their PR.

[newmuis]

+1 to Joey's concerns about the web UI, but otherwise LGTM.

(also +1 to Joey's stance on two spaces after a period, but I guess we can just force two spaces after a period upstream in the Prettier repo to have a wide impact 😄)

[rsimha]

My comment about two spaces after the period was mostly in jest

Phew :)

I don't suppose one of the PR checks we run could automatically commit a prettified form if the PR was generated using the web UI?

We considered auto-adding a prettified commit on Travis when we introduced this for JS files. (See design review notes and eventual PR #21212). We decided that it's preferable for developers to learn to prettify their own PRs instead of relying on Travis to do it automatically. I think the same rationale should apply for non-JS files. There might be an initial learning curve, but I'm confident that developers will get used to running the auto-fix command on their own.

My concern would be that I make a non-pretty change in the web UI, then the next person editing the doc prettifies the doc and gets a lot of unrelated changes in their PR.

As thing stand today, non-pretty PRs will be blocked by Travis whether they were created with the web-UI or with the regular command line workflow. I acknowledge this could be a point of friction. I don't think it's feasible / secure to allow Travis to push commits to branches on ampproject/amphtml (branch protection rules only allow reviewers and committers to do so). And finally, this is our standard for JS changes made via the web-UI, so I'd argue that the same standard should be applied for non-JS changes.

also +1 to Joey's stance on two spaces after a period, but I guess we can just force two spaces after a period upstream in the Prettier repo to have a wide impact :smile:

Prettier uses the remark-parse package to process markdown files. The only options it supports today are proseWrap (auto wrapping long paragraphs for easy reading) and singleQuote (single instead of double quotes). I haven't overridden either of these settings, but happy to if @CrystalOnScript or @mrjoro would like.

I'm sure the remark-parse maintainers will accept a PR that changes default spacing between sentences without any discussion or controversy ;)

[jeffjose]

+1 from my side. Always a fan of outsourcing formatting and style to tools and let humans focus on the content.

[rsimha]

The responses so far have been encouraging :) I'll experiment with the options and put together a formal PR with these changes and add everyone who commented on this issue as a reviewer. Meanwhile, more comments are welcome. Thanks everyone!

[rsimha]

Update: Just sent out #25182 for review. It uses prettier's default markdown formatting with no overridden settings.

[rcebulko]

A bit late to the party, but I support this. Agree with jeffjose re: outsourcing formatting and style. Any minor oddities will probably be offset by the advantages of standardizing the files.

[mrjoro]

And finally, this is our standard for JS changes made via the web-UI, so I'd argue that the same standard should be applied for non-JS changes.

I'm not sure I agree with this. Small documentation changes tend to be lighter-weight and easier to make, and something anyone can do without even having any knowledge of AMP or even web development (e.g. someone notices some incorrect grammar and wants to get a fix in). It's also hard enough to encourage people to improve our documentation--I don't want to raise the bar too high for these contributions.

Will the error message tell the author exactly what fix to make? If I'm a new contributor and make a change in the web UI that fails the prettify check, will they know exactly what they need to do to fix it?

[rcebulko]

I'd be in favor of enabling autofix, but not making Prettier a blocking check, so if someone makes edits in the UI they can but eventually it'll get formatted

[rsimha]

@mrjoro Thanks for raising the point about not wanting to create unnecessary friction for occasional contributors. I agree 100%.

Will the error message tell the author exactly what fix to make? If I'm a new contributor and make a change in the web UI that fails the prettify check, will they know exactly what they need to do to fix it?

This is an excellent idea. Let's say I'm able to enhance gulp prettify so that if a non-JS change isn't formatted correctly, the failing check prints out exactly what is wrong with the text, and maybe even prints a corrected version that the author can simply copy-paste into their editor. Would that alleviate the problem enough to make this whole effort worthwhile?

[rsimha]

Here's what printing suggested fixes will look like. #25182 has been updated with this functionality.

[mrjoro]

Looks good to me!

[rcebulko]

Nice!

[rsimha]

This is now done. For a full list of changes, see the PR description and final notes.