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

Prototype: npm run diff #6862

Closed
wants to merge 1 commit into from
Closed

Prototype: npm run diff #6862

wants to merge 1 commit into from

Conversation

foolip
Copy link
Collaborator

@foolip foolip commented Oct 8, 2020

No description provided.

@github-actions github-actions bot added dependencies ⛓️ Pull requests that update a dependency package or file. infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. scripts 📜 Issues or pull requests regarding the scripts in scripts/. labels Oct 8, 2020
@foolip foolip force-pushed the diff branch 2 times, most recently from 3232b60 to 6af15ce Compare November 4, 2020 09:17
@saschanaz
Copy link
Contributor

saschanaz commented Nov 7, 2020

It seems this currently requires a separate physical clone of BCD. How about using some git command to load previous state, if it's possible?

Edit: maybe git merge-base HEAD master, git diff COMMIT --name-only and git show COMMIT:FILENAME

@foolip
Copy link
Collaborator Author

foolip commented Nov 9, 2020

This in fact requires two additional copies of BCD, or at least that's how I've been using it. In a CI setup, my thinking was to first check out the target branch and save the state of BCD there into a JSON file, and then check out the head branch merged with the target branch, treating that as the RHS. This will be easy enough to make work.

For local use, however, it would be nice to be able to just run npm run diff and show a summary of the local changes. The problem with this is that if any JavaScript has changed, then the interpretation of JSON files might have changed, and it might not make sense to interpret the JSON files from another checkout using the JavaScript of this checkout. So far that mode, I think it would have to fail or print a warning if anything other than JSON files has changed.

Accepting that limitation, then a bunch of git commands can indeed get the state to compare to.

@ddbeck
Copy link
Collaborator

ddbeck commented Nov 12, 2020

The problem with this is that if any JavaScript has changed, then the interpretation of JSON files might have changed, and it might not make sense to interpret the JSON files from another checkout using the JavaScript of this checkout. So far that mode, I think it would have to fail or print a warning if anything other than JSON files has changed.

Perhaps if we were careful, going forward, we could use the package version to guard against this sort of thing? e.g., bake in an assumption that npm run diff can only act on the same major or major.minor version?

@foolip
Copy link
Collaborator Author

foolip commented Nov 24, 2020

@ddbeck yep, I think that would work.

For transparency, I don't know when I can make time to work on this, so if someone has all the pieces in their head and wants to go implement it, don't let this WIP PR stand in your way.

I think that https://github.com/mdn/yari/tree/master/client/src/document/ingredients/browser-compatibility-table is the new BCD table rendering for Yari, but I don't know if it can be used standalone.

@saschanaz saschanaz mentioned this pull request Nov 24, 2020
2 tasks
@saschanaz
Copy link
Contributor

Thanks for the update, I implemented the git way in #7486. Still text-only.

@foolip
Copy link
Collaborator Author

foolip commented Nov 25, 2020

Thanks @saschanaz! I'll keep this open for the time being since I use it every day myself, but once the scaffolding is there I might rework some of this on top of your script.

@queengooborg
Copy link
Collaborator

#7486 has now been merged -- thanks for getting the ball rolling on this script!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies ⛓️ Pull requests that update a dependency package or file. infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. scripts 📜 Issues or pull requests regarding the scripts in scripts/.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants