-
Notifications
You must be signed in to change notification settings - Fork 28
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
Adding github action to validate contextual survey json #104
Conversation
@jayoung-lee whenever you get a chance to review, this PR will essentially ensure that future updates to the context survey json will be valid and won't break |
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.
Great idea to add this!
A few minor drive-by comments to help with future readability/maintainability. I didn't cover too much as I know this is more of a one-off verification script.
Consider also adding a simple analysis workflow to help stay on top of future deprecations or warnings.
Co-authored-by: Parker Lougheed <parlough@gmail.com>
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.
I love the extra cleanup after you ran analysis!
Three more minor suggestions to be consistent with the rest of your code:
As for the analysis question: I would probably add it as a separate job in the same file, so it doesn't ever block the actual validation. Would something like this work? analyze:
runs-on: ubuntu-latest
defaults:
run:
working-directory: surveys/validator
steps:
- uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9
- uses: dart-lang/setup-dart@d6a63dab3335f427404425de0fbfed4686d93c4f
- name: Verify formatting
run: dart format --output=none --set-exit-if-changed .
- name: Analyze Dart files
run: dart analyze --fatal-infos |
I accidentally deleted your comment. I'm so sorry...I meant to delete one of mine. 🙇 Thanks for addressing my comments, I appreciate it! |
Yep! This seems to work as expected, and looks like both jobs ran in my latest commit |
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.
Looks great, thanks for making those adjustments!
It'll be nice to have this verification.
Reference issue:
This will add validation for the json file to ensure the data is correct
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.