Change equalise_attributes to optionally return deleted attr. (#3528) #4357
Conversation
SciTools-assistant
added
the
Blocked: CLA needed
larsbarring
force-pushed
the
return_deleted_attrs
branch
from
b240b60 to
b5d0e29
Compare
|
@larsbarring Fancy signing our CLA? Then we can get your contribution across the line 👍 |
SciTools-assistant
removed
the
Blocked: CLA needed
There was a problem hiding this comment.
@larsbarring Awesome first pull-request!
Thanks for taking the time to do this, it's really appreciated... and the first of many to come, I hope 😉
Just a couple of comments to service, and then this is looking really great.
Also, for bonus credits, do you fancy writing a simple unit test to cover your change in behaviour? If you have time for that then I'd suggest that you add your unit test in lib/iris/tests/unit/util/test_equalise_attributes.py. If you don't have time, then that's totally cool - I'll follow-up this PR with another PR adding test coverage (but I'll @ mention you on the PR so that you can see what I've done, just for reference) 👍
Finally, and most importantly, let's highlight your awesome contribution in the whatsnew. Simply update the docs/src/whatsnew/latest.rst at the end of the "✨ Features" section, with something along the lines of:
#. `@larsbarring`_ updated :func`~iris.util.equalise_attributes` to return a list of dictionaries
containing the attributes removed from each :class:`~iris.cube.Cube`. (:pull:`4357`)
Wordsmith it as you see fit. Then also add an rst reference to link to your GitHub user, down around line 155 i.e.,
.. _@tinyendian: https://github.com/tinyendian
.. _@larsbarring: https://github.com/larsbarring
Nice one 🍻
@larsbarring We're always looking to improve our documentation, for users and developers. If we were to put effort into making this part of the docs easier to understand/follow (and let's be honest, it really should be) could we garner some honest feedback from you, if it's not too much trouble? (BTW it's totally fine to say that you're too busy 😄) |
larsbarring
force-pushed
the
return_deleted_attrs
branch
from
b5d0e29 to
d4edd32
Compare
|
@larsbarring Thanks for the prompt changes 🎉 There's a minor doctest failure elsewhere associated with this change, so what I'll do is make a pull-request on your pull-request with the necessary changes to resolve that... plus I'll add a unit test to cover ourselves (since I'm in this space). I'll work on pushing that to you now... |
Well, as I'm not a software developer it would take quite some time to wrap my head around all these technicalities, already now I am outside my comfort zone, which means that ...
... your help is much appreciated. |
add unit test with minor fixes
🚀 Pull Request
My very first one:
Description
Change util.equalise_attributes to optionally return the deleted attributes as a list of dicts in an argument.
Closes #3528
Consult Iris pull request check list
which I am afraid I find a bit difficult to understand/follow...