Skip to content

add missing compiler options, add a test to help prevent this in future #839

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 7, 2025
Merged

add missing compiler options, add a test to help prevent this in future #839

merged 2 commits into from
May 7, 2025

Conversation

srabraham
Copy link
Contributor

@srabraham srabraham commented Apr 30, 2025 •
edited by jakebailey
Loading

This is a more comprehensive fix than #838. Future work would involve doing the same sort of tests for non-Tristate types.

This isn't exactly a pretty test, but if we take for a given that the big switch-case needs to exist as is, then you're better off having a nasty test like this than not having it.

Fixes #842

@srabraham srabraham mentioned this pull request Apr 30, 2025
Closed
@srabraham
Copy link
Contributor Author

@microsoft-github-policy-service agree

@srabraham srabraham force-pushed the srabraham-2025-04-30-parsingtest branch from 3366712 to ff3642a Compare April 30, 2025 17:56
@srabraham srabraham mentioned this pull request May 1, 2025
Closed
jakebailey
jakebailey requested changes May 7, 2025
View reviewed changes
@jakebailey
Copy link
Member

For the test, we may want to instead just loop over OptionsDeclarations instead.

@srabraham srabraham force-pushed the srabraham-2025-04-30-parsingtest branch 3 times, most recently from b28dc25 to 09790dc Compare May 7, 2025 17:49
@srabraham
Copy link
Contributor Author

For the test, we may want to instead just loop over OptionsDeclarations instead.

I spent some time in the code trying to figure out what you mean by this, but I couldn't get there

@jakebailey
Copy link
Member

For the test, we may want to instead just loop over OptionsDeclarations instead.

I spent some time in the code trying to figure out what you mean by this, but I couldn't get there

That's fair. I can send a follow-up PR to do this, building on your test.

@srabraham
Copy link
Contributor Author

srabraham commented May 7, 2025 •
edited
Loading

For the test, we may want to instead just loop over OptionsDeclarations instead.

I spent some time in the code trying to figure out what you mean by this, but I couldn't get there

That's fair. I can send a follow-up PR to do this, building on your test.

Thanks! Yeah, this was just meant to be a starting point for you all. Presumably you'd want tests for types other than tristates too. Go ahead and merge this if it makes sense.

I'm eager to switch over to tsgo in my own work. Thanks for your efforts!

EDIT: fixing tests...

@srabraham srabraham force-pushed the srabraham-2025-04-30-parsingtest branch 2 times, most recently from 96fffc3 to 962fbdd Compare May 7, 2025 17:59
@srabraham
Copy link
Contributor Author

srabraham commented May 7, 2025 •
edited
Loading

I think the tests/formatting should work now

EDIT: looking again...

@srabraham
add missing compiler options, add a test to help prevent this in future
322fe04 
This is a more comprehensive fix than #838. Future work would involve
doing the same sort of tests for non-Tristate types.
@srabraham srabraham force-pushed the srabraham-2025-04-30-parsingtest branch from 962fbdd to 322fe04 Compare May 7, 2025 18:29
@srabraham
Copy link
Contributor Author

Thanks for your patience. I'm still cloning the submodules locally, which are apparently required in order to run the particular failing test in this PR. Sigh...

@jakebailey
Copy link
Member

FYI, no need to force push; we squash on merge so your history can be as bad as you want it. The force pushing really only makes it hard to see what's changing when reviewing.

@srabraham
Copy link
Contributor Author

FYI, no need to force push; we squash on merge so your history can be as bad as you want it. The force pushing really only makes it hard to see what's changing when reviewing.

Ah, true. Yeah, bad habit from a past job

@jakebailey
Copy link
Member

Once you have cloned the submodule, you should be able to do hereby test && hereby baseline-accept and commit the changes.

@srabraham
update baseline
ab8dd41 
whoa that's a lot of diff
@srabraham
Copy link
Contributor Author

Once you have cloned the submodule, you should be able to do hereby test && hereby baseline-accept and commit the changes.

done, but wow, that just made so much diff I assumed it was wrong

@srabraham
Copy link
Contributor Author

Wunderbar! Please go ahead and merge at your leisure (looks like I can't do so myself). Thank you again

@jakebailey jakebailey added this pull request to the merge queue May 7, 2025
Merged via the queue into microsoft:main with commit 50a70ad May 7, 2025
22 checks passed
@srabraham srabraham deleted the srabraham-2025-04-30-parsingtest branch May 7, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakebailey jakebailey jakebailey approved these changes

@gabrielluizsf gabrielluizsf gabrielluizsf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some CompilerOptions don't work due to deficient switch-case in parsinghelpers.go
3 participants
@srabraham @jakebailey @gabrielluizsf