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

Add mirroring script #4729

Merged
merged 97 commits into from
Mar 24, 2020
Merged

Add mirroring script #4729

merged 97 commits into from
Mar 24, 2020

Conversation

queengooborg
Copy link
Contributor

@queengooborg queengooborg commented Aug 30, 2019

This PR is to aid in the efforts of #4719. It adds a new script designed to help eliminate inconsistencies and mirror data between browsers. The script started off as a carbon copy of the Python script I had written, but since has been enhanced with numerous new features. For example, command line arguments that allow the choice of a specific source browser, and to allow for forcing a mirroring on compatibility statements that aren't true or null. Furthermore, rather than using a static mapping of browser, a function has been written to dynamically retrieve said mappings, by finding the matching engine and the first release with an equal or greater version number, or false if one cannot be found.

This script also contains a more complex mirroring for Chromium-based Edge, to append the existing data rather than overwrite everything. (See #5214 (comment) for a description of the logic.) To use this Chromium-based mirroring, run npm run mirror edge [feature_or_file] -- --source=chrome --modify=always.

This script has undergone lots of field testing and has already created various merged PRs. While most of the bugs have been eliminated, there may still be a few lurking around in the corners. Let me know if you find a bug and I'll fix it right away!

Planned features after initial merge:

  • Mirror only with browser updates (i.e. Opera 68 came out, so mirror only data involving Chrome 81) -- this is planned after the initial merge due to the complexity of such a feature

@queengooborg queengooborg added infra Infrastructure issues (npm, GitHub Actions, releases) of this project docs Issues or pull requests regarding the documentation of this project. labels Aug 30, 2019
@queengooborg queengooborg requested a review from Elchi3 as a code owner August 30, 2019 02:45
@ghost ghost added the dependencies Pull requests that update a dependency package or file. label Sep 29, 2019
@Elchi3
Copy link
Member

Elchi3 commented Oct 16, 2019

Hey @AdaRoseCannon, you might be interested in this PR.
I haven't had the time to review it completely yet. However, @vinyldarkscratch has improved it whenever we've hit problems. Let us know if you have thoughts.

@mdn mdn deleted a comment from brown22jb Oct 25, 2019
scripts/mirror.js Outdated Show resolved Hide resolved
docs/contributing.md Outdated Show resolved Hide resolved
@Elchi3
Copy link
Member

Elchi3 commented Mar 23, 2020

I’d certainly love to figure out a programmatic way to at least warn about potentially invalid notes!

I think it would be a great improvement if the mirroring script would also print out a report of notes (ideally with links to the files and lines of the notes), so that the PR author can paste that report into the PR description and manually double check all notes reported before submitting the PR. This can be a follow-up after the initial landing of this project, though.

@queengooborg
Copy link
Contributor Author

Review comments have been addressed, thank you!

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Some more suggestions to the markdown. Sorry, tech writer reviewing here :)
(I didn't run prettier, but I hope the suggestions pass, might need to correct if not)

docs/contributing.md Outdated Show resolved Hide resolved
docs/contributing.md Outdated Show resolved Hide resolved
@Elchi3
Copy link
Member

Elchi3 commented Mar 24, 2020

Review comments have been addressed, thank you!

Thank you so much for moving things around! This is so much better! I'm quite happy with how it is now.

This script has undergone lots of field testing and has already created various merged PRs.

This is very true. I have now reviewed each bump function and I think they all look good so far. The one for Edge is a bit more complex, but this is also the one we've paid a lot of attention to when we recently updated all the Edge values and it seemed to do the right thing.

So, I think we can merge this, but should still continue to pay attention to the PRs generated by this script. Having this merged and "officially" supported is not a blind guarantee that generated data is always correct. To any reviewer: Please especially review notes and corner cases (version_removed, prefixes, flags, array statements and such things come to mind). The mirroring script does seem to do a good job for all of these things, but you never know basically. Of course we will fix bugs as we see them, and I think this script is now tidied up to allow extending it further as needed.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

I think I found a bug with the refactoring.

scripts/mirror.js Outdated Show resolved Hide resolved
@Elchi3 Elchi3 removed the dependencies Pull requests that update a dependency package or file. label Mar 24, 2020
@Elchi3
Copy link
Member

Elchi3 commented Mar 24, 2020

This works! If we can get the suggestions to the markdown file in, then I will merge this \o/

Co-Authored-By: Florian Scholz <[email protected]>
@ghost ghost added the dependencies Pull requests that update a dependency package or file. label Mar 24, 2020
@queengooborg queengooborg removed the dependencies Pull requests that update a dependency package or file. label Mar 24, 2020
@queengooborg
Copy link
Contributor Author

Oops, didn't see the documentation suggestions -- applied!

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thank you for 97 commits of patience, many months of field testing and getting this over the finish line now! This will be sooo enormously helpful! Cheers! 🎉 🥇 💯

@Elchi3 Elchi3 merged commit f196d15 into mdn:master Mar 24, 2020
@queengooborg queengooborg deleted the scripts/mirror branch March 24, 2020 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues or pull requests regarding the documentation of this project. infra Infrastructure issues (npm, GitHub Actions, releases) of this project scripts Issues or pull requests regarding the scripts in scripts/.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a mirroring script for future mirror migrations
3 participants