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

Remove errant underscore in AABB abbreviation #255

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mmoult
Copy link

@mmoult mmoult commented Feb 14, 2025

AABB stands for Axis-Aligned Bounding Box. In one of the ray tracing flag enumerations, it is pluralized as AABBs. This confused the shouty snake case generation.

There was already a special handling for NaN, add another case for AABBs and refactor the handling into its own function in utils. This compares to what has been done prior for converting strings into Ident.

Update the autogen files to use the new naming rules.

Format all code, which made small edits to code I didn't otherwise modify.

AABB stands for Axis-Aligned Bounding Box. In one of the ray tracing
flag enumerations, it is pluralized as AABBs. This confused the shouty
snake case generation.

There was already a special handling for NaN, add another case for AABBs
and refactor the handling into its own function in utils. This compares
to what has been done prior for converting strings into Ident.

Update the autogen files to use the new naming rules.

Format all code, which made small edits to code I didn't otherwise
modify.
Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Nice, thanks for commonizing it too!

It is probably unfeasible to match on a few input strings and capitalizing them by hand, right?

@mmoult
Copy link
Author

mmoult commented Feb 14, 2025

Depends on how you define unfeasible I guess. However, I think the replace after conversion approach (which I extended with another case) is more robust. Here are three potential strategies I see (in ascending order of my preference):

  1. Keep a list of strings which heck cannot handle and their expected results. (I think this is what you mean by "capitalizing them by hand", but correct me if I am wrong). This would work, but it would require us to update the list more frequently than the other approaches and may include duplicate info (only the 'NaN' part needs to be specially handled, but all other parts would be enumerated too, for example).
  2. Replace known abbreviations then feed into heck. For example, 'NaN' -> 'Nan' and 'AABBs' -> 'Aabbs'. This should work too, but requires more thought to give heck an input we know it will accept (and therefore, we are more dependent on heck's behavior). Also, requires a replacement at runtime for instances of all known abbreviations, even if heck changes in the future to support them.
  3. [Current] Feed the input into heck then replace known errors. I think post-processing is more predictable because there is no black box between our handling and the output. Also, the inverse situation described in point 2: if heck changes to handle AABBs, for example, then extra replacements we have will be unused. Assuming heck does the replacement more efficiently than our checking code, this would be ideal.

If you feel strongly about using another approach (including any I didn't enumerate), I could change the patch to do that, but I think it is best as-is.

Resolve the clippy and format errors to pass the CI. Also, replace a use
of to_snake_case()...uppercase() with shouty_snake_case.
@mmoult
Copy link
Author

mmoult commented Feb 21, 2025

What is next? I can be patient if people need more time to review- I just want to make sure that it isn't waiting on me.

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