-
Notifications
You must be signed in to change notification settings - Fork 3
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
_dynamic_info_firecrest_version
updated with new development
#60
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
+ Coverage 79.43% 82.43% +2.99%
==========================================
Files 4 4
Lines 671 683 +12
==========================================
+ Hits 533 563 +30
+ Misses 138 120 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these comments still up do date with this PR?
I am a bit confused how the function _dynamic_info_firecrest_version
works. You specify a version by the value, but if it is 0 then it tries to to retrieve the version dynamically. I guess that it is 0
and not None
confuses me, but fine. Maybe can you add a bit doc to it because value
is not such a meaningful name?
Why is the echo only done in case it is dynamically retrieved? Maybe you can add a comment there.
Thanks a lot, @agoscinski for the review!
yes, they are from the past, removed.
Changed to
Just to prevent verbosity. Usually when inputs are valid during setting up/configuring a computer in aiida, it remains quite. |
Looks clear to me, I just simplified the pytest a little bit, check out my commit |
for more information, see https://pre-commit.ci
Very good @agoscinski, thanks a lot! |
/status/parameters
endpoint of firecrest api is now updated with version value.We fetch this way and drop the previous work around that was implemented.
Fixes:
#43