-
Notifications
You must be signed in to change notification settings - Fork 283
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
metadata string reduction #4040
Conversation
@rcomer Hey, thanks for pushing this 👍 I'm about to make a similar change to |
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.
@rcomer Awesome, thanks! 😄
Just a couple of suggestions... also is you wanted to add some specific tests to exercise this, then that would be great 👍
docs/src/whatsnew/latest.rst
Outdated
@@ -48,6 +48,9 @@ This document explains the changes made to Iris for this release | |||
representation of the 64-bit non-cryptographic hash of the object using the | |||
extremely fast `xxhash`_ hashing algorithm. (:pull:`4020`) | |||
|
|||
#. `@rcomer`_ implemented a string method for metadata classes, so printing |
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.
Perhaps explicitly clarify, with string
-> __str__
?
lib/iris/common/metadata.py
Outdated
field_strings = [] | ||
for field in self._fields: | ||
value = getattr(self, field) | ||
if value is not None: |
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.
@rcomer I think that we could be even more liberal here and ignore empty strings perhaps, but definitely and empty dict
for the attributes
member.
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.
It took me longer than I care to admit to figure out the logic for this 😆
Hi @bjlittle, I've addressed those specific comments. Do you have anything particular in mind for tests? I haven't come up with any prime candidate for "this might go wrong", so was just thinking of checking the method works without crashing for one example out of each subclass. It would be good to know that's the Right Way Forward before embarking on that though. |
I can confirm that this method gets called indirectly by 60 other tests because, well, I broke them! |
@rcomer I think this is totally fine TBH. I may add two I'll push that separately, if you fancy reviewing and merged? |
Sweet. Thanks @bjlittle! |
🚀 Pull Request
Description
Suppose I make, for example, a coordinate, and print it
I get info about what it contains:
However, if I just print the metadata,
I see all the unset stuff too, which I'm not really interested in:
For differences, it's hard to see at a glance what is actually different:
With my branch, these outputs become:
Note that, if you do want to see all the unset metadata, you can still use the
repr
. We could potentially also skip empty attributes dictionaries for thestr
.Does something like this need tests? I had a peek at the coord string tests, but they seem to be focussed on getting date formats right.
Consult Iris pull request check list