fixes#47 Implement ability to query fields used on a worksheet #54
Conversation
…from <metadata-record>
|
This is a large review so I'll need some time to wrap my head around it (Nice find on the hidden bug in missing fields though!) Leaving some superficial comments on my first pass |
| </window> | ||
| </windows> | ||
| <thumbnails> | ||
| <thumbnail height='104' name='Sheet 1' width='144'> |
There was a problem hiding this comment.
Clear out the thumbnails, I think they'll just get regenerated as we iterate on these in the future and it keeps the diff a little more managable.
| @@ -64,3 +64,24 @@ def __getitem__(self, key): | |||
| key = self._indexes['caption'][key] | |||
|
|
|||
| return dict.__getitem__(self, key) | |||
|
|
|||
|
|
|||
| class PredicatedDictionary(object): | |||
There was a problem hiding this comment.
I think the code is here is fine, but I'm a little fuzzy on the need for a new type -- would a regular dictionary or data structure do?
It just makes the code read a little more complicated for new contributors. Not a strong objection, mostly curiosity.
There was a problem hiding this comment.
bah, this was left over from the previous approach. I'll remove it.
| @@ -33,7 +33,7 @@ def _get_metadata_xml_for_field(root_xml, field_name): | |||
|
|
|||
|
|
|||
| def _is_used_by_worksheet(names, field): | |||
| return any((True for y in names if y in field.worksheets)) | |||
| return any((y for y in names if y in field.worksheets)) | |||
There was a problem hiding this comment.
You can murder even more ()'s -- there's an implicit generator inside the fucntion call to any, so it can just be any(i for i in iterable if condition)
|
|
||
|
|
||
| class FieldDictionary(MultiLookupDict): | ||
| def found_in(self, name): |
There was a problem hiding this comment.
And let the bikeshedding begin!
found_in?
used_in?
used_by?
on_some_level_of_detail_somewhere_in?
There was a problem hiding this comment.
yeah found_in seems a bit not right to me. I think used_by hits the right level of obviousness, though maybe we need "used_by_worksheet" to be specific that this only works on worksheets (eventually a worksheet object, but for now, just the name)
There was a problem hiding this comment.
used_by_sheet() feels good to me (Or whatever our docs call a 'sheet' that is only a 'sheet' and not a dashboard or story)
|
🚀🚀🚀 (LGTM) I'm struggling with the basestring thing, I can't put my finger on why (I might prefer explicit py2/py3 checking but I may also just need sleep). I'll submit a PR if the 'better' way ever hits me. |
No description provided.