Skip to content

Conversation

@ratmice
Copy link
Collaborator

@ratmice ratmice commented May 22, 2025

I don't think we have any way to test this, so I manually tested by modifying one of the examples.

@ratmice
Copy link
Collaborator Author

ratmice commented May 22, 2025

Oh I should probably say more:

This one follows from the conversation at the end of #571

It flips the allow_missing_tokens_from_parser default value to false but changes the meaning to produce a warning.

The values here are implemented slightly differently than the defaults for CTParserBuilder.
In CTLexerBuilder the warnings_are_errors defaults to false while it is defaulting to true in CTParserBuilder.

As such it will now print a warning, but not produce an error.

@ltratt ltratt added this pull request to the merge queue May 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 22, 2025
}

has_unallowed_missing = self.warnings_are_errors;
has_unallowed_missing |= self.warnings_are_errors;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case where has_unallowed_missing = true and self.warnings_are_errors = false this would,
unset the has_unallowed_missing value back to false.

This was caught by lrpar/cttests/src/ctfails/missing.test

@ltratt
Copy link
Member

ltratt commented May 22, 2025

Please squash.

@ratmice ratmice force-pushed the ctlexerbuilder_warnings branch from d011477 to b41c03f Compare May 22, 2025 15:31
@ratmice
Copy link
Collaborator Author

ratmice commented May 22, 2025

Squashed.

@ltratt ltratt added this pull request to the merge queue May 22, 2025
Merged via the queue into softdevteam:master with commit fe3c354 May 22, 2025
2 checks passed
@ratmice ratmice deleted the ctlexerbuilder_warnings branch May 22, 2025 15:56
@ratmice
Copy link
Collaborator Author

ratmice commented May 22, 2025

So, unless anything comes up I believe that concludes everything I had intended to work on for this series of changes.

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