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

Corrected except KeyError to except AttributeError in TrajectoryData node methods. #5015

Merged
merged 22 commits into from
Oct 2, 2021

Conversation

Crivella
Copy link
Contributor

@Crivella Crivella commented Jul 8, 2021

In the TrajectoryData node there are several try checks on the get_attibute method, which except a KeyError in order then pick a default value/action.
KeyError should be changed to AttributeError as this is what the get_attribute method raises when the attribute is not found.

@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #5015 (e3fb7cf) into develop (97c3229) will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5015      +/-   ##
===========================================
+ Coverage    80.89%   80.91%   +0.03%     
===========================================
  Files          536      536              
  Lines        37019    37019              
===========================================
+ Hits         29944    29952       +8     
+ Misses        7075     7067       -8     
Flag Coverage Δ
django 75.77% <0.00%> (+0.03%) ⬆️
sqlalchemy 74.91% <0.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/orm/nodes/data/array/trajectory.py 46.35% <0.00%> (+1.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97c3229...e3fb7cf. Read the comment docs.

@ramirezfranciscof
Copy link
Member

Hey @Crivella ! Thanks for the contribution and sorry for the long wait!

I see that Codecov is complaining that there are not test for this, probably this contributed to us not noticing when this broke down. Would you be up for adding those? I would add a file in test/orm/data/test_array.py and just add three functions that trigger each of the raises and checks that the requested outputs are consistent.

@Crivella
Copy link
Contributor Author

Crivella commented Oct 1, 2021

HI @ramirezfranciscof , I have added the test that checks if the same try_except statement in the show_mpl_* functions behave properly when the attribute are not found.
I don't think i can check the variable directly as it is internally set andused by the function

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Thanks @Crivella ! Sorry the delay but I was looking if there was a way to monkeypatch plot_positions_XYZ to write to a file or something like that and test that instead of the direct raising of get_attribute, which is not optimal but I guess is the best we have so far.

Anyways, I think this can go in now and we can later improve moving to pytest or being more precise with the testing, but great base material!

@ramirezfranciscof ramirezfranciscof merged commit 2c5cc6f into aiidateam:develop Oct 2, 2021
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.

None yet

4 participants