ENH: add ability to test examples PR against PR in bids-specification #515
Conversation
Allow validate_datasets workflow to re-run when PR description is edited, enabling dynamic detection of bids-specification-pr references added after PR creation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Fix missing closing bracket in matrix JSON output - Fix variable name mismatch (PR_BODY_NUM vs PR_NUM) - Fix regex pattern for PR detection (escape sequences) - Fix bash conditional syntax (use || instead of 'or') 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Use heredoc to safely handle PR body content that may contain: - Quotes (single and double) - Newlines and multiline text - Special bash characters 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
Yep, there temporarily changes the workflow. This is what this tries to avoid and make it more explicit and so it could just be all handled via explicit metadata in description and then merge |
|
Sounds good. |
yarikoptic
added
enhancement
bids-validator
There was a problem hiding this comment.
The current process is to prevent non-dev runs from testing new examples, and use validator and schema from releated PRs in dev. Instead of adding a new bids-pr run, I think it would be helpful just to automate the changes to dev.
If you want the run name to be clearly different from dev, then I would replace dev with the other name in the matrix, but have the new name use identical steps to dev.
As to triggers, I don't really like the false positive rate of these triggers. I wonder if we could use a label trigger. Maybe the job could remove the trigger, but we could at least remove and re-add the label to re-trigger.
WDYT?
| <!-- Fill out or delete as appropriate --> | ||
|
|
||
| - Needs testing against a bids-specification-pr: NUMBER-or-URL | ||
| - TODOs: | ||
| - [ ] ... |
There was a problem hiding this comment.
I would expect a BEP PR to evolve, possibly starting with some examples before a spec or validator PR is written, but then adding them in as we get closer to completion. To that end, I would suggest leaving permanent fields with a default value of NONE.
| <!-- Fill out or delete as appropriate --> | |
| - Needs testing against a bids-specification-pr: NUMBER-or-URL | |
| - TODOs: | |
| - [ ] ... | |
| <!-- New examples often relate to in-progress changes to the specification or validator. Link to the relevant PRs, if any. --> | |
| - BIDS Specification PR: NONE | |
| - BIDS Validator PR: NONE | |
| - TODOs: | |
| - [ ] ... |
| cat << 'EOF' > /tmp/pr_body.txt | ||
| ${{ github.event.pull_request.body }} | ||
| EOF | ||
| PR_NUM=$(grep -oP 'bids-specification-pr:\s*(https://github\.com/bids-standard/bids-specification/pulls?/)*\K[0-9]+' /tmp/pr_body.txt | head -1 || true) |
There was a problem hiding this comment.
I would expect people to use the <org>/<repo>#<pr> syntax in some cases. I'm not sure if GitHub automatically rewrites it as a URL for pull_request.body or not, but you may want to account for that.
| if [ -n "${{ steps.find-pr.outputs.pr_number }}" ]; then | ||
| EXTRA_ITEM=', "bids-pr"' | ||
| fi | ||
| echo "matrix=[\"stable\", \"main\", \"dev\", \"legacy\"${EXTRA_ITEM}]" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
I would either deactivate dev or just update dev steps to use your environment variables.
| - name: Install BIDS validator (main) | ||
| if: matrix.bids-validator == 'main' | ||
| if: matrix.bids-validator == 'main' || matrix.bids-validator == 'bids-pr' |
There was a problem hiding this comment.
In general, dev tracks unreleased changes to the schema, while main tracks the latest released version of BIDS. PRs against the spec will mostly aim for dev.
2 participants
Many PRs are developed to accompany WiP BEP PRs. It is beneficial to establish testing against likely a modified BIDS schema in those PRs. In this solution we should expand matrix of runs with
bids-prrun against that arbitrary PR if entered in a line withbids-specification-pr:.TODOs
should run correctly ifdecided not to bother since would lead on github to local PR etc#number(TODO)