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.

Sign up for GitHub

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

Jump to bottom

Added type hinting for weight_windows.py #2467

Merged
merged 5 commits into from
Apr 19, 2023
Merged

Added type hinting for weight_windows.py #2467

merged 5 commits into from
Apr 19, 2023

Conversation

christinacai123
Copy link
Contributor

No description provided.

shimwell
shimwell reviewed Apr 13, 2023
View reviewed changes
openmc/weight_windows.py Outdated Show resolved Hide resolved
christinacai123
christinacai123 commented Apr 13, 2023
View reviewed changes
Copy link
Contributor Author

@christinacai123 christinacai123 left a comment

Choose a reason for hiding this comment

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

fixed space on line 23

@christinacai123 @shimwell
Apply suggestions from code review
966f3e5 
Co-authored-by: Jonathan Shimwell <[email protected]>
christinacai123
christinacai123 commented Apr 13, 2023
View reviewed changes
Copy link
Contributor Author

@christinacai123 christinacai123 left a comment

Choose a reason for hiding this comment

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

fixed spacing on line 23

paulromano
paulromano requested changes Apr 13, 2023
View reviewed changes
openmc/weight_windows.py Outdated Show resolved Hide resolved
openmc/weight_windows.py Outdated Show resolved Hide resolved
openmc/weight_windows.py Outdated Show resolved Hide resolved
openmc/weight_windows.py Outdated Show resolved Hide resolved
openmc/weight_windows.py Outdated Show resolved Hide resolved
openmc/weight_windows.py Outdated Show resolved Hide resolved
openmc/weight_windows.py Outdated Show resolved Hide resolved
openmc/weight_windows.py Outdated Show resolved Hide resolved
christinacai123
christinacai123 commented Apr 15, 2023
View reviewed changes
Copy link
Contributor Author

@christinacai123 christinacai123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Added suggested changes

@christinacai123 christinacai123 changed the title added type hinting for weight_windows.py Added type hinting for weight_windows.py Apr 15, 2023
@christinacai123
Copy link
Contributor Author

christinacai123 commented Apr 15, 2023 •
edited
Loading

We failed one of the checks and were wondering if you had any advice on how to fix this?

@paulromano
Copy link
Contributor

@christinacai123 The test failure was actually unrelated to your changes and should be fixed by #2481. I've just retriggered the CI and hopefully it will pass this time around 🤞

@christinacai123
Copy link
Contributor Author

Unfortunately, I am still failing the same check. Could you try retriggering the CI again?

paulromano
paulromano reviewed Apr 18, 2023
View reviewed changes
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Ah, it looks like you need to make a new commit on the branch for it to pick up updates from the develop branch in CI. I've just made a trivial suggestion that you can commit and hopefully that should do the trick.

openmc/weight_windows.py Outdated Show resolved Hide resolved
@paulromano paulromano added the Merging Soon PR will be merged in < 24 hrs if no further comments are made. label Apr 18, 2023
@paulromano paulromano merged commit e0abe73 into openmc-dev:develop Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shimwell shimwell shimwell left review comments

@paulromano paulromano paulromano approved these changes

Assignees
No one assigned
Labels
Merging Soon PR will be merged in < 24 hrs if no further comments are made.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@christinacai123 @paulromano @shimwell