Conversation
|
apparently Position unit tests are missing for |
|
this should not be passing before the corresponding PR is merged in Cyclus (cyclus/cyclus#1510) |
|
@gonuke I updated this. |
Build Status Report - 29642a4 - 2025-06-15 15:25:29 +0000Build
|
|
These failures are because we need cyclus/cyclus#1510 to be merged before these changes make any sense. |
|
Now that cyclus/cyclus#1510 has been merged, does this need another look? @bam241 @gonuke @katyhuff |
|
I believe one should first rerun the ci. I can push an empty commit if needed :) |
|
I triggered the CI to see if that's working as expected |
|
CI seems to be working, moved it into ready to review (from draft) |
katyhuff
left a comment
katyhuff
left a comment
There was a problem hiding this comment.
This looks good. I have a fussy set of style edits that add a space between function input parameters. Accepting these changes would be nice, but is not required for merging, so I will mark this "Approve" as far as I'm concerned (as long as CI tests are continuing to pass).
gonuke
left a comment
gonuke
left a comment
There was a problem hiding this comment.
The initialization of the injected variables needs to be reviewed and updated (as necessary) to the patterns that we've used elsewhere
| latitude(0.0), | ||
| longitude(0.0), | ||
| coordinates(latitude, longitude) {} |
There was a problem hiding this comment.
I think we need to review/reconfirm the way that we do initialization of variables that are injected. We don't initialize other variables quite this way, I don't think.
@katyhuff suggestions Co-authored-by: Katy Huff <katyhuff@users.noreply.github.com>
|
Hi @bam241, I spoke with @gonuke this morning about this PR and he mentioned that you had told him previously that you might not have a ton of time for it. If it's alright with you, then, I'd love to volunteer to try to get this over the finish line. I would, of course, request your input for review (though you need not give it if you're swamped) but before I went ahead and started changing stuff I wanted to ping you and double check that I wouldn't be stepping on any toes by doing so. Let me know whenever if that works, and if not no worries! |
|
@dean-krueger find by me, I sent you an invite to my cycamore repo, so you can keep this open. If you prefer opening an new PR, do as you wish :) I am not "swamped" but I didn't work on this for long time, it is time consuming to go back into this :) If you need anything, ping me |
github-project-automation
bot
moved this from Backlog
to Done
in March 2024 Hack-a-thon
4 participants
This follows cyclus/cyclus#1510.
simplifying the call to toolkit::Position