-
Notifications
You must be signed in to change notification settings - Fork 11
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
del_properties
methods, and ignore_properties
keyword to cf.unique_constructs
#241
Conversation
Codecov Report
@@ Coverage Diff @@
## main #241 +/- ##
==========================================
+ Coverage 87.50% 87.52% +0.03%
==========================================
Files 124 124
Lines 12740 12764 +24
==========================================
+ Hits 11147 11171 +24
Misses 1593 1593
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hi David, since you haven't merged are you wanting and awaiting a review on this, and then for NCAS-CMS/cf-python#598 too? If so I will assign myself and get it done shortly. |
Hi Sadie - thanks for asking, but I think it's best to hold off for now, as it turns out Grenville is still testing it. I'll request the review formally when he's done (in a week or two?). |
Oh sure, I wasn't aware of that so thanks for the clarification. I'll wait until it pops up in my notifications. |
Ready now - sooner than expected. See also NCAS-CMS/cf-python#598. |
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.
Some minor comments raised, but otherwise all good, with the new testing nice and thorough. Please merge when ready.
|
||
If *ignore_properties* has been set then *copy* is ignored | ||
and deep copies are always returned, even if | ||
*ignore_properties* is an empty sequence. |
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.
💯 Very clear and concise.
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
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.
All minor feedback addressed, so good to go and merge (in case a re-confirmation is helpful).
Fixes #240