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

Fix bug due to poor naming of QResources attribute #32

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

gpetretto
Copy link
Contributor

The kwargs attribute of QResources is not handled correctly by monty during serialization.
Renaming it to qkwargs

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f960941) 62.43% compared to head (f7c8df1) 43.35%.
Report is 1 commits behind head on develop.

❗ Current head f7c8df1 differs from pull request most recent head 3c2ab3d. Consider uploading reports for the commit 3c2ab3d to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##           develop      #32       +/-   ##
============================================
- Coverage    62.43%   43.35%   -19.09%     
============================================
  Files           15       15               
  Lines         1118     1128       +10     
  Branches       216      222        +6     
============================================
- Hits           698      489      -209     
- Misses         418      611      +193     
- Partials         2       28       +26     
Files Coverage Δ
src/qtoolkit/core/data_objects.py 84.75% <100.00%> (-15.25%) ⬇️
src/qtoolkit/io/pbs.py 19.90% <0.00%> (ø)
src/qtoolkit/io/slurm.py 22.13% <0.00%> (-55.18%) ⬇️

... and 3 files with indirect coverage changes

Copy link
Member

@davidwaroquiers davidwaroquiers left a comment

Choose a reason for hiding this comment

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

Good to me except (maybe) for naming ? (see line comments) I let you decide if you think qkwargs is fine or else, it is not a super crucial point anyway.

src/qtoolkit/core/data_objects.py Outdated Show resolved Hide resolved
src/qtoolkit/core/data_objects.py Outdated Show resolved Hide resolved
@davidwaroquiers
Copy link
Member

All good to me!
About the timedelta, in the end I guess you went with removing it then? (if we see a need in future, we can also add it again)
Can be merged!

@gpetretto
Copy link
Contributor Author

Yes, I also removed the timedelta type for time_limit. Adding it back with a better support will be possible at a later time.

@gpetretto gpetretto merged commit df939ba into Matgenix:develop Jan 30, 2024
5 checks passed
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