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

Fix MDN URLs in api folder #5203

Merged
merged 6 commits into from
Dec 1, 2019
Merged

Fix MDN URLs in api folder #5203

merged 6 commits into from
Dec 1, 2019

Conversation

bershanskiy
Copy link
Contributor

@bershanskiy bershanskiy commented Nov 25, 2019

Summary

Fix MDN URLs in api folder.
Currently, mdn_urls are full of mischief like linking Magnetometer.x to Magnetometer.y. :)

This linter finds all URLs that don't match either of two patterns:

https://developer.mozilla.org/docs/Web/API/feature/subfeature
https://developer.mozilla.org/docs/Web/API/feature#subfeature

There are still a few non-standard URLs, which I think should just be whitelisted individually. (Details below.)

  1. CSSMathNegate.values was renamed into CSSMathNegate.value because that is the standard name.
  2. Headers.headers renamed to Headers.Headers because it is a constructor.
  3. MediaKeyStatusMap.iterator renamed to MediaKeyStatusMap.@@iterator to match existing naming convention.

Related

#5201 is a tracking bug for all MDN URLs.
#5195 was the original issue that demonstrated existence of incorrect URLs.

A checklist to help your pull request get merged faster:

  • Summarize your changes
  • Data: link to resources that verify support information (such as browser's docs, changelogs, source control, bug trackers, and tests)
  • Data: if you tested something, describe how you tested with details like browser and version
  • Review the results of the linter and fix problems reported (If you need help, please ask in a comment!)
  • Link to related issues or pull requests, if any

@ghost ghost added data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. labels Nov 25, 2019
@queengooborg
Copy link
Collaborator

Hey @bershanskiy, thanks for taking initiative on this one! I agree, this is a big problem that we should have resolved. A couple of things, though:

  1. Can we split off all of the changes into a separate PR? This will help us during the review process.
  2. Moreso, I think this should follow the bulk update process we've defined, since this will be such a large number of updates.
  3. I think this can apply for more than just the API data; CSS, JavaScript, HTML, etc. features may also experience this issue. Can we add support for other folders as well, and make this a per-file test, rather than testing the entire folder? (This will also fix an issue I noticed where this linter file checks the entire API folder every file, instead of just once per run of npm test.)

@bershanskiy
Copy link
Contributor Author

Can we split off all of the changes into a separate PR? This will help us during the review process.

Do you mean split the data changes into a separate PR?

Moreso, I think this should follow the bulk update process we've defined, since this will be such a large number of updates.

I'm not sure it's possible to make a script to update these files because there are too many edge cases. E.g., when to use slash and when to use #? This depends mostly on MDN article structure, so a script would need to fetch external resource and make a choice based on it -- not very elegant. Also, some URLs reference section titles in English article, so a script will need to fetch the article in a specific locale (English) and check for a specific section existence -- pretty ugly as well.

I think this can apply for more than just the API data

This is true. I started with API data just to try out the method. I agree: the whole data should be linted, ideally.

This will also fix an issue I noticed where this linter file checks the entire API folder every file, instead of just once per run of npm test.)

Ooops... Good catch!

Another thing I noticed is: many descriptions (e.g., for events) are linted to have a very specific format suitable for English locals only. If the format is known in advance, why bother saving it in the data in the first place? Why not just provide a function that creates these descriptions and, may be, insert them into release "builds" of the data sent to npm?

@queengooborg
Copy link
Collaborator

Do you mean split the data changes into a separate PR?

Ack, forgot to say "data" -- that's what I mean, yeah!

I'm not sure it's possible to make a script to update these files because there are too many edge cases.

Hmm, yeah, you do have a point... This makes me wonder if we should have a bulk update process for changes that are better to be performed manually, rather than by a script -- maybe we should just keep it broken up into small PR chunks?

Another thing I noticed is: many descriptions (e.g., for events) are linted to have a very specific format suitable for English locals only. If the format is known in advance, why bother saving it in the data in the first place? Why not just provide a function that creates these descriptions and, may be, insert them into release "builds" of the data sent to npm?

This is a really good point, and one we should look into more. Overall, I feel that this boils down to localization, which is an ongoing discussion in #114.

@bershanskiy
Copy link
Contributor Author

@vinyldarkscratch Let's split this PR into multiple. I dropped non-data changes to focus this commit on api/ directory only. Is this PR small enough to review?

@bershanskiy
Copy link
Contributor Author

FYI, all changes are basically one-liners. Some files have large diffs because the change is in the dictionary key and after sorting the order of features changes so diffs look rather large.

@queengooborg queengooborg changed the title Linter for mdn_url Fix MDN URLs in api folder Nov 29, 2019
@queengooborg queengooborg removed the linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. label Nov 29, 2019
@queengooborg
Copy link
Collaborator

Thanks for splitting it up! Yeah, this looks great, and we should be able to review it pretty soon. We should also take the description from this and apply it to a PR that contains the linter script (don't worry if it fails without the data changes; that just shows it's working), and write up a new description -- one that doesn't have the "fixes" keyword. Can we do that?

Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Looking good, just got a couple out-of-scope changes we should probably separate into its own PR!

api/CSSMathNegate.json Outdated Show resolved Hide resolved
api/DOMHighResTimestamp.json Outdated Show resolved Hide resolved
api/Document.json Outdated Show resolved Hide resolved
api/Headers.json Outdated Show resolved Hide resolved
api/MediaKeyStatusMap.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Thanks for all of your hard work on this, this is looking good to me!

@queengooborg queengooborg merged commit 13bd6cf into mdn:master Dec 1, 2019
@bershanskiy bershanskiy deleted the linter-url branch December 1, 2019 17:18
@Elchi3 Elchi3 removed their request for review January 8, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants