Linter for MDN URL structure #5273

bershanskiy · 2019-12-05T04:05:36Z

Summary

This is linter from #5201. Currently, it is very experimental because I would like to fix as many actual bugs before committing to a particular linter implementation.

Questions for bike-shedding:

Should linter enforce capitalization?
MDN uses case-insensitive URLs (serves the same content regardless of capitalization), but that would weaken the linter.
...

TODO list:

Linter is still incomplete (it's in the middle of rewrite from first iteration to the second iteration).
Need to find all special cases when an API links to a "Using [something]" instead of the dry "[parent].[child]" docs pages
Avoid multiple linter runs for the same data
Need to resolve all PRs in Linter for mdn_url #5201 before linter can be finalized.

CC @vinyldarkscratch @sideshowbarker

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

bershanskiy · 2019-12-05T04:11:19Z

@vinyldarkscratch @sideshowbarker
FYI, this is in a very early stage (that's why I made PR only after @vinyldarkscratch asked for it) and there are very many false positives which I'm aware of. This is intentional because I wanted to fix all actual bugs first before making any test cases for data that is subject to change.

I don't think we should merge any linter until all URL issues are fixed, and I think #2537 won't be fixed this year.

queengooborg · 2019-12-05T04:12:59Z

Thanks for creating this PR, and for taking initiative on this! This has already revealed so many issues along the way, primarily due to copy-pastes of existing data that weren't fully updated. To answer your question regarding capitalization enforcement, since MDN is case-insensitive I don't think it really matters much, but I'd say that there's no reason not to enforce it.

I'm wondering if this new test file should also go into test-links.js, since it's...well, testing links. However, before that, I'm hoping to get #5187 merged (to prepare for a little project I have to add automatic link fixing.)

Since we're still working on fixing up the URLs, and this linter is still experimental, I'm gonna go ahead and add the "not ready" label to let the other maintainers know. 😉

bershanskiy · 2019-12-05T04:32:47Z

To answer your question regarding capitalization enforcement, ... there's no reason not to enforce it.

That's my thought process exactly.

I'm wondering if this new test file should also go into test-links.js, since it's...well, testing links.

I created a separate file because it does not share any logic with that file. test-links.js reads in the data as a sting and puts it through processLinks() function that finds long URL string in that string and suggests short URLs. As such, test-links.js does not need the concept of data hierarchy, so uses strings for efficiency. On the other hand, this test uses actual objects and heavily relies on data hierarchy. This test generates regular expressions for acceptable URLs for each level in the data tree and matches mdn_urls against those. Therefore, the overlap is rather minimal.

FYI, I do care about performance too and try to avoid unnecessary costly (complex) regular expressions. I'll share final benchmark data later when patch is more complete.

Edit: typo.

sideshowbarker · 2019-12-06T05:16:44Z

I wonder if this linter should have support for checking if each mdn_url is a 404 or not — and expose that support as a non-default option. (I’m not familiar in general with the existing linter code here, so I don’t know if any of the existing linting features have optional behaviors that can be enabled…)

If such 404-checking were added, I think it would be need to be as a non-default option, due to the fact it’s going to be making a network request for every single mdn_url value in every JSON file in the repo, and waiting for each response, and that’s gonna take a long time to complete…

sideshowbarker · 2019-12-06T05:30:15Z

Should linter enforce capitalization?

Yes, I think it would be best if it did

MDN uses case-insensitive URLs (serves the same content regardless of capitalization), but that would weaken the linter.

So yeah then what the linter would be helping to enforce in this case is consistency in styling of the URLs (beyond just strictly checking for complete mismatches). But I think doing that level of style checking would be consistent with the purpose of the linter (and with linting in general).

sideshowbarker

I reviewed this (initial code) and tested it and found it works as expected (great!). I have no changes to suggest at this point, but I will also happily further updates/refinements

bershanskiy · 2019-12-06T05:43:16Z

I wonder if this linter should have support for checking if each mdn_url is a 404 or not

I agree that URL checker would be nice (not only 404 but any non-200 code, especially 301 and 302). However, I don't think we should add it in this PR because it'll already be pretty large (300+ lines of code).

I’m not familiar in general with the existing linter code here

As far as I know, #5228 introduces broken URL check for shortened URLs (aka URLs to bug reports most of which are in the notes to the compat data). I don't really know how it'll work out, so for now I just plan to wait it out.

If such 404-checking were added, I think it would be need to be as a non-default option, due to the fact it’s going to be making a network request for every single mdn_url value in every JSON file in the repo, and waiting for each response, and that’s gonna take a long time to complete…

It definitely won't make a lot of requests by default. However, I do want to lint all new URLs, so I want to add "incremental" linting -- lint only files that were changed in the PR and check only URLs that were changed in the PR. Specifically:

Ask git for names of files that differ between the current PR source HEAD and mdn/master
Lint only those files (with all linters)
For each file that changed, extract URLs from the mdn/master version (which are presumed to be good) and the current HEAD version; lint only the new URLs, if there are any.

This approach reduces lint time from ~8 seconds to less than a second for most commits I work on. (I like this because I'm really impatient).

Of course, once in a while (e.g., before each release or once a day) someone/something should do a complete (non-incremental) lint to sanity-check.

Edit: tyop.

bershanskiy · 2019-12-06T05:47:17Z

I reviewed this (initial code) and tested it and found it works as expected (great!). I have no changes to suggest at this point, but I will also happily further updates/refinements

I'm pushing all the most recent stuff to https://github.com/bershanskiy/browser-compat-data/tree/linter-mdn_url2 because I need to pull/merge/rebase from mdn/master all the time to incorporate the latest fixes. I'll push all the good stuff to this branch once it's in a presentable state.

queengooborg · 2019-12-06T14:58:34Z

I wonder if this linter should have support for checking if each mdn_url is a 404 or not

I agree that URL checker would be nice (not only 404 but any non-200 code, especially 301 and 302). However, I don't think we should add it in this PR because it'll already be pretty large (300+ lines of code).

I’m not familiar in general with the existing linter code here

As far as I know, #5228 introduces broken URL check for shortened URLs (aka URLs to bug reports most of which are in the notes to the compat data). I don't really know how it'll work out, so for now I just plan to wait it out.

Don't worry, #5228 covers all of that. It checks every URL within BCD (mdn_url, spec, notes...) for a non-2xx code, and reports back. 😉

It definitely won't make a lot of requests by default. However, I do want to lint all new URLs, so I want to add "incremental" linting -- lint only files that were changed in the PR and check only URLs that were changed in the PR.

This certainly brings up an interesting way to optimize linter time! I'd say that this is something we should probably implement for not just this linter, but all of them. Let's get a discussion going for this -- can you open up an issue so we can get that going?

bershanskiy · 2020-03-08T11:34:31Z

test/linter/test-mdn_url.js

+  /** @type {Identifier} */
+  // TODO(bershanskiy): remove require, if applicable
+  // See https://github.com/mdn/browser-compat-data/pull/5795
+  const data = require(filename);


Wait for #5795.

bershanskiy · 2020-03-08T11:40:36Z

I have updated the linter and rebased the PR to pull in changes from master. This PR is still work-in-progress and should wait for:

Migration 007: Remove all mdn_url 404s #5297 which removes some URLs caught by this linter
refactor: Avoid require(…) of JSON files #5795 which might replace require() with JSON.parse() and fs (if refactor: Avoid require(…) of JSON files #5795 is merged, I'll update this PR)
Pick linter strictness vs. shortness
Currently, most of URL linter (200 lines) is just declarations of what URL patterns should be. We can either use fewer general patterns resulting in shorter code but "weaker" linter or use more specific patters resulting in "stronger" linter but longer code. I personally prefer stronger linter that even declares the preffered URL right away.
Fix some apparent bugs in BCD

ddbeck · 2020-03-09T11:29:09Z

Thanks for this summary @bershanskiy. That makes it a lot easier for me to prioritize reviews.

bershanskiy · 2020-03-10T13:48:07Z

Tracking list of URLs that might need to be updated:
(I'll update this list as I work on linter.)

`LocalFileSystem` and `LocalFileSystemSync`

These two pages contain documentation of their methods (as a section of the pages) instead of having dedicated pages for each method. I believe it's better to move each method to a separate page, following the pattern established by the rest of MDN. Thoughts?

Linter output for api\LocalFileSystem.json and api\LocalFileSystemSync.json:

× api\LocalFileSystem.json
              Actual: "https://developer.mozilla.org/docs/Web/API/LocalFileSystem#resolveLocalFileSystemURL"
              Expected: "https://developer.mozilla.org/docs/Web/API/LocalFileSystem/resolveLocalFileSystemURL"
              Actual: "https://developer.mozilla.org/docs/Web/API/LocalFileSystem#requestFileSystem"
              Expected: "https://developer.mozilla.org/docs/Web/API/LocalFileSystem/requestFileSystem"
× api\LocalFileSystemSync.json
              Actual: "https://developer.mozilla.org/docs/Web/API/LocalFileSystemSync#resolveLocalFileSystemSyncURL()"
              Expected: "https://developer.mozilla.org/docs/Web/API/LocalFileSystemSync/resolveLocalFileSystemSyncURL"
              Actual: "https://developer.mozilla.org/docs/Web/API/LocalFileSystemSync#requestFileSystem"
              Expected: "https://developer.mozilla.org/docs/Web/API/LocalFileSystemSync/requestFileSystemSync"

WebExtensions `devtools.*` APIs

All WebExtension DevTools APIs have patterns with slugs devtools.something instead of devtools/something.

E.g.: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/devtools.panels
Also, there is no dedicated page like https://developer.chrome.com/extensions/devtools, only https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/user_interface/devtools_panels

I propose:

Create https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/devtools
Make all applicable pages children of it, that us move all devtools.something to devtools/something.

bershanskiy · 2020-08-23T16:22:09Z

This PR is stale, I'll open a new one once I update the patch.

bershanskiy requested a review from Elchi3 as a code owner December 5, 2019 04:05

ghost added the linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. label Dec 5, 2019

bershanskiy mentioned this pull request Dec 5, 2019

Linter for mdn_url #5201

Closed

queengooborg added the not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label Dec 5, 2019

This was referenced Dec 6, 2019

Remove broken MDN URLs: webextensions.api.events #5279

Merged

Update MDN URLs: css.selectors.-moz-system-metric #5280

Merged

Fixup MDN URLs: Web/API/Headers.json #5282

Merged

Fix MDN URL: api/MediaSession.json #5283

Merged

sideshowbarker approved these changes Dec 6, 2019

View reviewed changes

queengooborg changed the title ~~Linter for mdn_url~~ Linter for MDN URL structure Feb 15, 2020

Linter for mdn_url

4ac2c9e

bershanskiy commented Mar 8, 2020

View reviewed changes

bershanskiy mentioned this pull request Mar 9, 2020

Update theme_experiment.json #5800

Merged

5 tasks

Support some more URL patterns

7942b9b

bershanskiy closed this Aug 23, 2020

Elchi3 removed their request for review August 26, 2020 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linter for MDN URL structure #5273

Linter for MDN URL structure #5273

bershanskiy commented Dec 5, 2019

bershanskiy commented Dec 5, 2019

queengooborg commented Dec 5, 2019

bershanskiy commented Dec 5, 2019 •

edited

Loading

sideshowbarker commented Dec 6, 2019

sideshowbarker commented Dec 6, 2019

sideshowbarker left a comment

bershanskiy commented Dec 6, 2019 •

edited

Loading

bershanskiy commented Dec 6, 2019

queengooborg commented Dec 6, 2019 •

edited

Loading

bershanskiy Mar 8, 2020

bershanskiy commented Mar 8, 2020

ddbeck commented Mar 9, 2020

bershanskiy commented Mar 10, 2020

bershanskiy commented Aug 23, 2020

Linter for MDN URL structure #5273

Linter for MDN URL structure #5273

Conversation

bershanskiy commented Dec 5, 2019

Summary

bershanskiy commented Dec 5, 2019

queengooborg commented Dec 5, 2019

bershanskiy commented Dec 5, 2019 • edited Loading

sideshowbarker commented Dec 6, 2019

sideshowbarker commented Dec 6, 2019

sideshowbarker left a comment

Choose a reason for hiding this comment

bershanskiy commented Dec 6, 2019 • edited Loading

bershanskiy commented Dec 6, 2019

queengooborg commented Dec 6, 2019 • edited Loading

bershanskiy Mar 8, 2020

Choose a reason for hiding this comment

bershanskiy commented Mar 8, 2020

ddbeck commented Mar 9, 2020

bershanskiy commented Mar 10, 2020

LocalFileSystem and LocalFileSystemSync

WebExtensions devtools.* APIs

bershanskiy commented Aug 23, 2020

bershanskiy commented Dec 5, 2019 •

edited

Loading

bershanskiy commented Dec 6, 2019 •

edited

Loading

queengooborg commented Dec 6, 2019 •

edited

Loading

`LocalFileSystem` and `LocalFileSystemSync`

WebExtensions `devtools.*` APIs