Skip to content
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

Improve test samples to catch more errors. #88

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

ysono
Copy link
Contributor

@ysono ysono commented Oct 18, 2024

This PR changes some samples to correctly catch some errors as intended.

I edited expected_results.json manually, without running generate_expected_results.py. Will .github/workflows/check_autogen_files.yml run for this PR?

One thing I left untouched: tests/chapter_6/valid/ternary_rh_binop.c: this test passes on solutions that prioritize either the logic or the conditional operator. Maybe using something other than || operator on the RHS would be better?

@nlsandler
Copy link
Owner

This is great, thank you! .github/workflows/check_autogen_files.yml will run for this PR, but it should pass - the change you made to expected_results.json looks identical to what the script would have changed.

Thanks for flagging the issue in ternary_rh_binop.c - I'll open a separate PR to fix that.

Copy link
Owner

@nlsandler nlsandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall - just one question and one spot where we need to update a comment to match your change

@ysono
Copy link
Contributor Author

ysono commented Oct 29, 2024

As with the other PR, please feel free to make amendments. I think you have far better context for how the final change should look like.

@nlsandler nlsandler merged commit 01c77e8 into nlsandler:main Oct 30, 2024
95 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants