Update copy_attribute plugin to include copying history attribute #2055
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2055 +/- ##
========================================
Coverage 98.39% 98.39%
========================================
Files 124 133 +9
Lines 12212 13117 +905
========================================
+ Hits 12016 12907 +891
- Misses 196 210 +14 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This looks good to me and all the tests are passing, with the exception of the one that always fails. There is one section that is failing CodeCov, I don't know if that needs fixing before merging.
I've left a few comments, they aren't necessary as they will work, they're just more Pythonic.
|
|
||
| plugin = CopyAttributes(attributes) | ||
| result = plugin.process(cube0, template_cube_2, template_cube) | ||
| assert type(result) is Cube |
There was a problem hiding this comment.
I don't think it really matters but it's probably better to use assert isinstance(result, Cube) here
| assert result.attributes["attribA"] == "tempA" | ||
| assert result.attributes["attribB"] == "tempB" | ||
| assert "attribC" not in result.attributes | ||
| assert id(result) == id(cube0) |
There was a problem hiding this comment.
If the id's are the same then they are the same object and you should use assert result is cube0
| assert type(result) is Cube | ||
| assert result.attributes["attribA"] == "tempA" | ||
| assert result.attributes["attribB"] == "tempB" | ||
| assert "attribC" not in result.attributes | ||
| assert id(result) == id(cube0) |
There was a problem hiding this comment.
Same pedantic comments here as above
| @@ -76,3 +124,45 @@ def test_copy_attributes_single_input(): | |||
| assert result.attributes["attribD"] == "valueD" | |||
| assert "attribC" not in result.attributes | |||
| assert id(result) == id(cube0) | |||
There was a problem hiding this comment.
assert result is cube0
There was a problem hiding this comment.
The changes look fine, they seem to do what you want them to do. The tests seem to check what you need them too as well. The tests all seem to pass so I'm happy with this. You will need to rebase before it can be merged due to CopyAttributes becoming CopyMetada and then I'll approve 👍
mspelman07
force-pushed
the
history_copy
branch
from
a893082 to
a263735
Compare
Addresses https://metoffice.atlassian.net/browse/EPPT-1717
This PR updates the ability of the copy attributes plugin to include an exception for copying the history attribute if there are multiple input template cubes. In this case the most recent time stamp is used.
I've also slightly altered its ability to take in multiple cubes. Originally you could provide 1 template cubes and the attributes were copied from this cube onto all other provided cubes. I've now changed this to take in 1 cube and multiple template cubes requiring that the template cube attributes we are copying all match.
Testing: