-
Notifications
You must be signed in to change notification settings - Fork 2k
Update the diff script to be smarter and diff against the merge base. #1990
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
Conversation
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)
scripts/diff.js, line 44 at r1 (raw file):
branchName = exec(`git rev-parse --abbrev-ref HEAD`).stdout.trim(); } console.log('commitSha: ', commitSha);
are the log statements useful (actionable) to keep?
scripts/diff.js, line 67 at r1 (raw file):
exec(`git fetch origin ${mergeBase}`); exec(`git checkout ${mergeBase}`); console.log('mergeBase: ', mergeBase);
useful to keep the log?
nsthorat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @dsmilkov)
scripts/diff.js, line 44 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
are the log statements useful (actionable) to keep?
spoke offline -- going to keep these because if the script goes wrong these are useful for debugging
scripts/diff.js, line 67 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
useful to keep the log?
see above
NOTE: do not update this PR's branch with master.
The diff script now clones and the current branch and commit as well as master rewinded to the merge base of the current branch and master to only find changes against where the branch was created (only true for branches on the main repo).
For forks, we diff against tensorflow/tfjs:master.
Test PRs:
Note that this PR is behind tfjs-node version bump from 1.2.8 => 1.2.9
This change is