Skip to content

Prototype: npm run diff#6862

Closed
foolip wants to merge 1 commit intomdn:mainfrom
foolip:diff
Closed

Prototype: npm run diff#6862
foolip wants to merge 1 commit intomdn:mainfrom
foolip:diff

Conversation

@foolip
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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

@foolip
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor

#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