Skip to content

Conversation

@ratmice
Copy link
Collaborator

@ratmice ratmice commented May 21, 2025

Here is take two of PR #570 this one shouldn't affect semver in any way because it ignores the duplication of data between between the rule_ids, and yacc_grammar field.

This just avoids the code duplication from my original patch without touching on that issue by tweaking things to be less like nimbleparse, and more like the code already in CTLexerBuilder.

We can perhaps reduce field duplication as a follow up, if it is something you would like to see just say so.
Perhaps using the later commits in #570 as a starting point (or not!)

cl.missing_from_lexer().map(|x| x.to_owned()),
cl.missing_from_parser().map(|x| x.to_owned()),
cl.missing_from_parser()
.map(|x| x.iter().map(|(n, _)| n.to_owned()).collect::<HashSet<_>>()),
Copy link
Collaborator Author

@ratmice ratmice May 21, 2025

Choose a reason for hiding this comment

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

Perhaps worth noting that this ugly bit is in deprecated code.

@ltratt
Copy link
Member

ltratt commented May 22, 2025

This one I like! I think we can merge this as-is?

@ratmice
Copy link
Collaborator Author

ratmice commented May 22, 2025

Sure, the only thing I that has come to mind after sleeping on it is whether we should print a warning when
allow_missing_tokens_from_parser and allow_missing_terms_in_lexer is false. I guess that is a change of behavior
and worth doing separately.

The thing to note is that allow_missing_terms_in_lexer defaults to false, so by default nothing gets printed.
I guess I would suggest if we want that to do it in a follow up.

CTLexerBuilder to date hasn't had any warnings, and so lacks all the machinery like warnings_are_errors and show_warnings, etc. I think if we want to add all of that it's best to do it in a follow up, and I'm not sure/opinionated on what the defaults should be.

@ltratt
Copy link
Member

ltratt commented May 22, 2025

Sure, the only thing I that has come to mind after sleeping on it is whether we should print a warning when
allow_missing_tokens_from_parser and allow_missing_terms_in_lexer is false. I guess that is a change of behavior and worth doing separately.

Agreed.

CTLexerBuilder to date hasn't had any warnings, and so lacks all the machinery like warnings_are_errors and show_warnings, etc. I think if we want to add all of that it's best to do it in a follow up, and I'm not sure/opinionated on what the defaults should be.

Can we follow the precedent in CTParserBuilder?

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

ratmice commented May 22, 2025

Sure, the only thing I that has come to mind after sleeping on it is whether we should print a warning when
allow_missing_tokens_from_parser and allow_missing_terms_in_lexer is false. I guess that is a change of behavior and worth doing separately.

Agreed.

CTLexerBuilder to date hasn't had any warnings, and so lacks all the machinery like warnings_are_errors and show_warnings, etc. I think if we want to add all of that it's best to do it in a follow up, and I'm not sure/opinionated on what the defaults should be.

Can we follow the precedent in CTParserBuilder?

We can, however CTParserBuilder defaults to warnings_are_errors and show_warnings,
so changing the default allow_missing_tokens_in_parser would jump from not being printed by default, to an error, which I'm not certain we want.

We might want to change allow_missing_tokens_in_lexer to false, but let the default for warnings_are_errors be false,
so it becomes a warning unless warnings_are_errors is explicitly set to true?

That would be more in line with the current CTLexerBuilder behavior while still printing something by default at least.
I don't think I have any strong opinion on error-vs-warn by default for this case though.

@ratmice ratmice deleted the missing_spanned_take2 branch May 22, 2025 13:10
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