🐛 Bug fix: check links test #26739
Merged
🐛 Bug fix: check links test #26739
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Before, links to files added by the PR would simply be skipped over in the search for dead links. Instead, by using relative links to markup files, all links are checked regardless. Closes #26713 |
rsimha
reviewed
Feb 11, 2020
There was a problem hiding this comment.
Yay, your first PR! 😃
Couple high-level comments here:
- This PR will break the common use-case where a PR adds a new
.mdfile and also adds a full, non-relative link to that file from another file in the repo. See check-links.js should allow for links that are within the PR #9628 (comment) for the original motivation behind ignoring links to files added by the PR. - Forcing all
amphtmldocumentation to use relative links is not something we should aim to do. There are hundreds of files with a few hundred non-relative links in our codebase right now.
To address this, I think a good compromise would be to forgive any non-relative links to files added by the same PR if the relative portion of that link is correct. (See comment below).
WDYT?
| @@ -134,10 +118,6 @@ function checkLinksInFile(file) { | |||
| } | |||
| let containsDeadLinks = false; | |||
| for (const {link, status, statusCode} of results) { | |||
| // Skip links to files that were introduced by the PR. | |||
| if (isLinkToFileIntroducedByPR(link)) { | |||
| continue; | |||
There was a problem hiding this comment.
After restoring all the lines deleted, instead of doing a continue here, maybe you could remove the https://github.com/ampproject/amphtml/blob/master/ prefix from the link (if it exists), normalize the rest of the path, and see if the link resolves correctly file exists in the PR branch?
Edit: Fixed prefix, altered suggested action.
ajwhatson
force-pushed
the
desktop_first_branch
branch
from
ea98b24
to
53b91d9
Compare
estherkim
approved these changes
Feb 20, 2020
build-system/tasks/check-links.js
Outdated
| @@ -134,9 +135,15 @@ function checkLinksInFile(file) { | |||
| } | |||
| let containsDeadLinks = false; | |||
| for (const {link, status, statusCode} of results) { | |||
| // Skip links to files that were introduced by the PR. | |||
| // Catch all files that were introduced by the PR. | |||
| if (isLinkToFileIntroducedByPR(link)) { | |||
There was a problem hiding this comment.
What if the link is a relative path?
Suggested change
| if (isLinkToFileIntroducedByPR(link)) { | |
| if (isLinkToFileIntroducedByPR(link) && status == 'dead') { |
|
Merged. Nice job on your first PR @ajwhatson 🎉 |
robinvanopstal
added a commit
to jungvonmatt/amphtml
that referenced
this pull request
Feb 24, 2020
* master: (41 commits) custom-element: Minor test improvements (ampproject#26923) amp-pixel: Minor test improvements (ampproject#26918) viewer: Minor test improvements (ampproject#26906) dom: Minor test improvements (ampproject#26913) amp-action: Support whitelist lookup in AmpDocShadow (ampproject#26684) ✨ Update amp-access-scroll (ampproject#26810) 🚀 Remove doc css and base css from ESM build (ampproject#26889) 📖 [amp-story-player] Initial docs (ampproject#26606) Amp consent restrict fullscreen prod flag (ampproject#26909) 📖 Clarify SXG duration minimum (ampproject#26890) Improve test vendor requests macros (ampproject#26828) 🚀 Move scroll left and top macros out of url-replacement-impl (ampproject#25594) Update consent string maximum size to 200 bytes (ampproject#26741) ✨[amp-story-player] Adds tap-to-next/previous story (ampproject#26865) update owners file with correct syntax (ampproject#26899) amp-sticky-ad: Fix unit test (ampproject#26855) Add performance metrics to README (ampproject#26891) 🐛 Bug fix: check links test (ampproject#26739) ✨Idealmedia uniq ad (ampproject#25838) 📦 Update dependency jsdom to v16.2.0 (ampproject#26591) ...
alanorozco
added a commit
that referenced
this pull request
Mar 12, 2020
This breaks due to reassigning `const`, introduced in #26739
alanorozco
added a commit
that referenced
this pull request
Mar 12, 2020
This breaks due to reassigning `const`, introduced in #26739
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.