Skip to content

🏗🚀 Refactor and significantly speed up gulp check-links#25235

Merged
rsimha merged 3 commits intoampproject:masterfrom
rsimha:2019-10-23-RefactorLinkChecker
Oct 24, 2019
Merged

🏗🚀 Refactor and significantly speed up gulp check-links#25235
rsimha merged 3 commits intoampproject:masterfrom
rsimha:2019-10-23-RefactorLinkChecker

Conversation

@rsimha
Copy link
Copy Markdown
Contributor

@rsimha rsimha commented Oct 24, 2019

This PR builds on #25226 and refactors / significantly speeds up gulp check-links

Highlights:

  • Moves common functionality like getFilesToCheck() into build-system/common/utils.js
  • Introduces the --files and --local_changes flags (--files now supports lists of globs)
  • Makes it an error to run gulp check-links on all files in the repo (there are multiple thousands of links, so we only run the link checker on files changed by a PR)
  • Uses the ignorePatterns option of markdown-link-check and deletes filterAllowedLinks() (addresses 🏗 Ignore markdown templates in link checker by convention #25226 (comment))
  • Caches gitDiffAddedNameOnlyMaster() in filesIntroducedByPr (significant speed up)
  • Processes and prints results for each .md file as soon as they are available (better UX, also faster)
  • Adds support for ignored links while reporting results (yellow dots)
  • Eliminates the use of BBPromise in favor of the native interface of markdown-link-check

Usage:

# Check all .md files changed in the local branch
gulp check-links --local_changes

# Check just the files specified
gulp check-links --files "build-system/**/*.md"

Example:

Screenshot from 2019-10-24 12-11-47

Follow up to #25226

@rsimha rsimha requested a review from alanorozco October 24, 2019 02:52
@rsimha rsimha self-assigned this Oct 24, 2019
@amp-owners-bot
Copy link
Copy Markdown

Hey @estherkim, these files were changed:

  • build-system/pr-check/checks.js

Comment thread build-system/common/utils.js Outdated
Copy link
Copy Markdown
Member

@alanorozco alanorozco left a comment

Choose a reason for hiding this comment

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

Are we sure we'd like to ignore build-system overall?

Comment thread build-system/tasks/check-links.js
Comment thread build-system/tasks/check-links.js Outdated
Comment thread build-system/common/utils.js Outdated
Comment thread build-system/tasks/check-links.js Outdated
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Oct 24, 2019

@alanorozco Can you post the rest of your comments in one go by clicking the "Start a review" button? Posting several comments one by one generates notification spam and makes it hard to collapse / group review comments.

And thanks for reviewing!

Copy link
Copy Markdown
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Comments addressed.

Comment thread build-system/common/utils.js Outdated
Comment thread build-system/tasks/check-links.js Outdated
Comment thread build-system/tasks/check-links.js Outdated
Comment thread build-system/tasks/check-links.js
Comment thread build-system/common/utils.js Outdated
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Oct 24, 2019

Are we sure we'd like to ignore build-system overall?

Not sure what this comment means. We do include build-system/. It's also possible to explicitly check the files in there (see screenshot in description).

Comment thread build-system/tasks/check-links.js
@kristoferbaxter
Copy link
Copy Markdown
Contributor

Nice set of changes! Thanks Raghu.

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Oct 24, 2019

Thanks for the reviews, everyone. All comments addressed. Merging now, so that #25182 can be unblocked.

@rsimha rsimha merged commit 5225036 into ampproject:master Oct 24, 2019
@rsimha rsimha deleted the 2019-10-23-RefactorLinkChecker branch October 24, 2019 17:00
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants