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

Replace "volid" with "vol_id" throughout ORANGE #1486

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

elliottbiondo
Copy link
Contributor

@elliottbiondo elliottbiondo commented Nov 5, 2024

This MR updates all files within ORANGE to use the form "vol_id" for variable names instead of "volid", for consistency with other opaque IDs such as universe_id, transform_id, level_id, etc.

Copy link

github-actions bot commented Nov 5, 2024

Test summary

 3 371 files   5 194 suites   3m 54s ⏱️
 1 577 tests  1 549 ✅ 28 💤 0 ❌
17 384 runs  17 318 ✅ 66 💤 0 ❌

Results for commit fa61437.

♻️ This comment has been updated with latest results.

@elliottbiondo elliottbiondo changed the title Use vol_id throughout for consistency Replace "volid" with "vol_id" throughout ORANGE Nov 5, 2024
@elliottbiondo
Copy link
Contributor Author

@sethrj RTR

Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Looks great. Do you want to do the same for univ_id, trans_id, ...? I think it would be a nice touch. Also like we have uid in there, there are a couple of vid for vol_id.

@elliottbiondo
Copy link
Contributor Author

Yeah I can do that too. I wasn't sure how far too go. I am thinking:

surf_id,
trans_id,
univ_id,

but only for local variable names. If I start changing APIs (geo.volume_id() to geo.vol_id()) the changes become more invasive. I also don't want to mess but the .jsons.

@sethrj
Copy link
Member

sethrj commented Nov 6, 2024

Yes, agreed. I think it's ok (and often better?) for API names to be spelled out. In fact, I read/saw recently a guideline that said the more global an identifier's scope, the longer/more descriptive the name should be; and conversely very local variables can be very short (think i in an inner loop, or v for volume id in a 3-line loop).

@sethrj sethrj added minor Minor internal changes or fixes orange Work on ORANGE geometry engine labels Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Minor internal changes or fixes orange Work on ORANGE geometry engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants