-
Notifications
You must be signed in to change notification settings - Fork 5
ENH: only run diffed content #49
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
base: main
Are you sure you want to change the base?
Conversation
test.sh
Outdated
if [ $# -gt 0 ]; then | ||
files_to_process="$@" | ||
if [[ "$1" == all ]]; then |
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.
if [[ "$1" == all ]]; then | |
if [[ "$1" == --all ]]; then |
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.
Maybe make --all
the default because it requires no further input for the user.
test.sh
Outdated
else | ||
files_to_process=$all_markdown_files | ||
# We only want to run tests for the notebooks we touched | ||
files_to_process=$(git fetch origin main --depth=1; git diff origin/main --name-only tutorials | grep .md) |
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.
If the usage is --changed <branch>
we can avoid hard-coding a specific branch name.
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.
Sounds good, and the we can hard-code origin and main on CI as those are set up there.
fyi: I'm stuck with the git logic a bit, to figure out a generic way for the arguments to pass on to git fetch/etc, as opposed to what we had to be run on CI only before. |
921f875
to
52287fb
Compare
I took this for a spin. Maybe we need two arguments:
so that we can |
Yes, that's the one I converged on yesterday, I think I just sent myself down to unnecessary rabbit holes during the summit of automatically finding base branches for the default case of not providing arguments (e.g. I want the tox file to be convenient both locally and on CI) |
(FYI: I'm also using this PR to test the redirect mess as it's still not 100% up and working) |
52287fb
to
3565257
Compare
OK, finally the redirector action works. I'm still puzzled why the context conditional didn't work, but leave that rabbit hole for another day. |
Cool. Testing this locally, I do see it successfully detecting local changes and only testing the changed notebooks. We just need to pull apart
|
There are some problems with this, e.g. I don;t have
origin
locally, and the logic didn't fully work with github URLs (nor I want to hardwire the repo link in the test file).So, do we want this to just be done in CI, but locally we test with all the files? I'm not fully certain what's the best approach.