Fix JSON validation to require the cows directory - #52
Conversation
|
|
🧪 PR Validation Results for #52❌ No JSON file found! Please include your .json file. |
|
|
|
Thanks so much for the contribution! I especially appreciate that you disclosed your AI use. It is very helpful context to have as a maintainer. I am busy attending a conference this/next week but will do my best to review this when I can! |
|
Hi @oohwooh — hope the conference went well. This PR is still current with |
oohwooh
left a comment
oohwooh
left a comment
There was a problem hiding this comment.
Thanks for your patience on my review!
Adding tests and exemptions to the validator for maintenance changes should be their own PRs instead of included in this one. You are welcome to open PRs for those additions, but please remove those changes from this PR so it's just the the cows directory fix.
I think the fix you identified is great, my only feedback on it is that it probably doesn't need it's own helper function since it's just one line.
Thanks for this PR! Once it's just the cows directory fix I think it is good to merge 😄
| if (isValidatorMaintenancePullRequest(files)) { | ||
| console.log("✅ Validator maintenance PR detected; cow submission checks skipped."); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
I think this is out of scope for this PR. I think it's a good idea for a feature but should be in a PR of it's own vs this one.
| @@ -0,0 +1,15 @@ | |||
| export function isCowJsonFile(filename) { | |||
There was a problem hiding this comment.
In my opinion, this doesn't need to be it's own function, it can be inline like on validate-pr L58: https://github.com/codeday/fantasy-cow/pull/52/changes#diff-0e0c95e0e9e8936d11f2fa26475f0476bf09ed38718744d8cf60c6212f3f8a72L49
This would make the code easier to skim and prevent any confusion of the name isCowJsonFile since that could imply that it does validation of the JSON when that isn't the point of the function
| return filename.startsWith("cows/") && filename.endsWith(".json"); | ||
| } | ||
|
|
||
| export function isValidatorMaintenancePullRequest(files) { |
There was a problem hiding this comment.
As mentioned in my other comment, this is out of scope for this PR.
If you're interested in making a different PR for this work, I think an easier way to detect if it's a maintenance PR is make a label that gets manually applied (and could even trigger a different set of GH actions, like running tests)
There was a problem hiding this comment.
I think a testing framework should be split into its own PR as well.
It would also be useful to have some tests where we run entire changesets through the flow and check validation outputs, we can look through past PRs for real-world examples to include in that suite.
@HanCotterell any thoughts on testing? I know you did a lot of testing by hand when developing this, what cases do you think would be most important to include?
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Run validator tests |
There was a problem hiding this comment.
Adding tests out of scope for this PR
| "description": "A small project that teaches how to create a first Pull Request.", | ||
| "type": "module", | ||
| "scripts": { | ||
| "test": "node --test .github/scripts/*.test.js" |
There was a problem hiding this comment.
Adding tests out of scope for this PR
| // --- Basic pre-checks --- | ||
| if (jsonFiles.length === 0) { | ||
| commentString += `\n---\n\n`; | ||
| commentString += "❌ No JSON file found! Please include your **<cow>.json** file.\n"; |
There was a problem hiding this comment.
Maybe it would make sense to update the error message to specify cows/cow.json to give people a starting point to figure out why the test failed if they submitted a JSON that is outside the cows folder.
It doesn't need to explain perfectly the issue, the philosophy for fantasy-cow is to mimic real-world PR workflows, which often times are opaque and wouldn't give this granular of feedback.
If you think it's better left as-is I'd also be okay with that, though!
2 participants
Summary
.jsonfiles undercows/as cow descriptionsCloses #49.
Testing
npm cinpm test(5 tests)node --check .github/scripts/validate-pr.jsnode --check .github/scripts/validation-helpers.jsnode --check .github/scripts/validation-helpers.test.jsgit diff --checkAI assistance disclosure
This fix, its regression tests, and this pull request description were prepared with OpenAI Codex under the contributor account owner’s direction. The submitted change was validated locally with the commands above.