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.

Sign up for GitHub

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

Jump to bottom

Mark font server URLs as not intended to work #10645

Merged
merged 2 commits into from Jul 28, 2017
Merged

Mark font server URLs as not intended to work #10645

merged 2 commits into from Jul 28, 2017

Conversation

robinpokorny
Copy link
Member

@robinpokorny robinpokorny commented Jul 26, 2017 •
edited

When changing spec/amp-html-format.md Trevis fails during check-links task. For example here: https://travis-ci.org/ampproject/amphtml/jobs/257735944#L757

screenshot 2017-07-26 18 15 55

These two font providers do not implement the root page and return 404 error.

Changes: All three font provider URLs are made non-clickable. Small improvements in check-links task.

Related-to #10644

@robinpokorny robinpokorny mentioned this pull request Jul 26, 2017
Merged
rsimha
rsimha reviewed Jul 27, 2017
View reviewed changes
build-system/tasks/check-links.js Outdated
@@ -137,6 +137,10 @@ function filterWhitelistedLinks(markdown) {
// Dailymotion link that is marked dead in Travis CI
filteredMarkdown = filteredMarkdown.replace(/https:\/\/developer.dailymotion.com\/player#player-parameters/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're making this change, would you mind breaking this long line into two by adding a new line after replace(?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing!

Which do you prefer?

filteredMarkdown = filteredMarkdown.replace(
  /https:\/\/developer.dailymotion.com\/player#player-parameters/g,'');

or (more consistent)

filteredMarkdown = filteredMarkdown.replace(
  /https:\/\/developer.dailymotion.com\/player#player-parameters/g,
  ''
);

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated with the first option.

@robinpokorny robinpokorny force-pushed the add-font-servers-to-whitelist branch from a1c1762 to c0668af Compare July 27, 2017 14:56
rsimha
rsimha suggested changes Jul 27, 2017
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment •
edited

Choose a reason for hiding this comment

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

Undoing my approval, with some new info. Let me know what you think. I'm happy to defer the final decision to you and / or @pbakaus

build-system/tasks/check-links.js Outdated
@@ -135,7 +135,12 @@ function filterWhitelistedLinks(markdown) {
filteredMarkdown = filteredMarkdown.replace(/<code>(.*?)<\/code>/g, '');

// Dailymotion link that is marked dead in Travis CI
filteredMarkdown = filteredMarkdown.replace(/https:\/\/developer.dailymotion.com\/player#player-parameters/g, '');
filteredMarkdown = filteredMarkdown.replace(
/https:\/\/developer.dailymotion.com\/player#player-parameters/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing this. For line continuations, we need 4 spaces of indent.

build-system/tasks/check-links.js Outdated

// Font providers CDNs that return 404 on the root
filteredMarkdown = filteredMarkdown.replace(/https:\/\/fast.fonts.net/g, '');
filteredMarkdown = filteredMarkdown.replace(/https:\/\/fonts.googleapis.com/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

I just thought about this further, and there is precedent for wrapping links that are not supposed to work within backticks. See #10585 (comment) for a discussion on this. (cc @pbakaus)

If you want a uniform look across all three links listed in this section, may I suggest that you put all of them within backticks, since they are origins and not actual URLs that need to be clicked?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Let's not format things as clickable URLs that aren't actually targets. I therefore think we shouldn't whitelist them.

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem, I wasn't sure what is the better solution :)

Thanks for comments!

@rsimha rsimha requested a review from pbakaus July 27, 2017 15:02
@robinpokorny robinpokorny force-pushed the add-font-servers-to-whitelist branch from c0668af to 1bd8f59 Compare July 27, 2017 21:33
pbakaus
pbakaus reviewed Jul 27, 2017
View reviewed changes
build-system/tasks/check-links.js Outdated
@@ -135,7 +135,12 @@ function filterWhitelistedLinks(markdown) {
filteredMarkdown = filteredMarkdown.replace(/<code>(.*?)<\/code>/g, '');

// Dailymotion link that is marked dead in Travis CI
filteredMarkdown = filteredMarkdown.replace(/https:\/\/developer.dailymotion.com\/player#player-parameters/g, '');
filteredMarkdown = filteredMarkdown.replace(
/https:\/\/developer.dailymotion.com\/player#player-parameters/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit unrelated to this PR in particular (issue existed before): Any reason why we're not escaping dots? These would match stuff like "https://fast fonts net".

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is an oversight. Fixed it in the whole file.

Copy link
Contributor

Choose a reason for hiding this comment

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

great thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this was an oversight on my part when I first wrote the link checker. Thanks for fixing.

@robinpokorny robinpokorny force-pushed the add-font-servers-to-whitelist branch from 1bd8f59 to 8b235a0 Compare July 27, 2017 21:39
@robinpokorny robinpokorny changed the title Add font servers to whitelist Mark font server URLs as not intended to work Jul 27, 2017
@robinpokorny
Copy link
Member Author

@rsimha-amp, @pbakaus please re-review :)

@rsimha
Copy link
Contributor

rsimha commented Jul 28, 2017

Thanks @robinpokorny for making the changes, and @pbakaus for reviewing.

@jridgewell jridgewell merged commit cd99bb1 into ampproject:master Jul 28, 2017
@robinpokorny robinpokorny deleted the add-font-servers-to-whitelist branch July 29, 2017 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha approved these changes

@pbakaus pbakaus pbakaus approved these changes

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@robinpokorny @rsimha @pbakaus @jridgewell @googlebot @ampprojectbot