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

🏗📖 Auto-format all markdown files in ampproject/amphtml with Prettier #25182

Merged
merged 7 commits into from
Oct 24, 2019
Merged

🏗📖 Auto-format all markdown files in ampproject/amphtml with Prettier #25182

merged 7 commits into from
Oct 24, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Oct 21, 2019

The AMP Project already uses Prettier to auto-format JS files and several non-JS files across the repo. This PR adds documentation files to the mix.

Highlights:

  • Includes .md files in the list of files that are checked by Prettier
  • Auto-formats all .md files in the repo by running gulp prettier --fix
  • Skips the broken links check for PRs that modify a large number of .md files
  • Prints suggested fixes when formatting errors are detected
  • Exempts GitHub issue templates (they use a custom header that's incompatible with Prettier)
  • Make extension generator (gulp make-extension) resilient to markdown file auto-fixing

Notes:

  • Most of these changes do not affect the rendered HTML. Exceptions are:
    • Attribute lists are changed to all-in-one-line if they all fit on one line
      image
    • Attribute lists are changed to one-per-line if they don't all fit on one line
      image
  • Once this PR is merged, .md files will have to satisfy Prettier's formatting requirements
  • A workflow to make your IDE auto-fix .md files on save is available here
  • An alternative is to manually run gulp prettify --local_changes --fix

Example of a failing check:

Fixes #25148

@rsimha
Copy link
Contributor Author

rsimha commented Oct 22, 2019

The link checker is reporting several seemingly broken links in our documentation ☹️ On taking a closer look, most of these failures are due to requests to amp.dev timing out, probably because of the sheer number of requests being made by the checker.

I don't think we should be running the broken links check for PRs that modify a large number (> 20) of markdown files, so I've skipped it for large PRs. The check will continue to run when a smaller number of files are touched.

@rsimha
Copy link
Contributor Author

rsimha commented Oct 22, 2019

I sanity checked several of the changes in this PR, and found no visible changes to the rendered HTML in most cases. This is now ready for review.

@cramforce cramforce removed their request for review October 22, 2019 19:55
@ampproject ampproject deleted a comment from codecov bot Oct 23, 2019
@rsimha
Copy link
Contributor Author

rsimha commented Oct 23, 2019

Rebased and added a function to print suggested fixes for badly formatted .md files.

Copy link
Member

@mrjoro mrjoro left a comment

Choose a reason for hiding this comment

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

overall changes look fine (with a few comments) though I admit to not reviewing all 483 files in detail :)

.github/ISSUE_TEMPLATE/bug-report.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/intent-to-ship--i2s-.md Outdated Show resolved Hide resolved
3p/README.md Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented Oct 23, 2019

Rebased, reverted changes to issue template headers, and exempted them from future checks.

@rsimha rsimha requested a review from mrjoro October 24, 2019 16:20
@rsimha
Copy link
Contributor Author

rsimha commented Oct 24, 2019

Some final notes:

  • Self-reviewed every file in contributing/, and most files in ads/, extensions/, etc.
  • Found and fixed a handful of existing bugs that were exposed by auto-formatting (tabs instead of spaces, bad formatting, etc.)
  • Added prettier-ignore overrides where required
  • Verified that the majority of .md files have no visible changes when rendered
  • Tested VSCode auto-fix-on-save workflow for .md files
  • Tested gulp prettify with --fix, --files, and --local_changes

Merging this now 🤞

@rsimha rsimha merged commit e33dd10 into ampproject:master Oct 24, 2019
@rsimha rsimha deleted the 2019-10-18-PrettifyMarkdown branch October 24, 2019 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shall we auto-format our .md files with Prettier?
3 participants