fixes#47 Implement ability to query fields used on a worksheet - #54
Conversation
Sorry, something went wrong.
…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 |
Sorry, something went wrong.
| </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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
bah, this was left over from the previous approach. I'll remove it.
Sorry, something went wrong.
|
|
||
| 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)
Sorry, something went wrong.
|
|
||
|
|
||
| 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?
Sorry, something went wrong.
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)
Sorry, something went wrong.
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)
Sorry, something went wrong.
|
🚀🚀🚀 (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. |
Sorry, something went wrong.
No description provided.