-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add check images script #292
Conversation
Also refactors compute_many_descriptors to use the newly generalized methods.
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.
The separated functions should be less attached to specific plugin implementations.
PIL.Image.open(io.BytesIO(dfe.get_bytes())) | ||
return True | ||
except IOError, ex: | ||
return False |
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.
Define a log = logging.getLogger(__name__)
and reinstate the removed warning here.
return False | ||
|
||
|
||
def is_valid_element(fp, valid_content_types=None, check_image=False): |
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.
This should take in a DataElement
input instead of a specific file path. This opens up the use of this function with things like URLs and stuff through other element implementations.
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.
Also unit test(s).
from smqtk.representation.data_element.file_element import DataFileElement | ||
|
||
|
||
def is_loadable_image(dfe): |
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.
This should move away from DataFileElement
input requirement because nothing in the body of the function requires file-specific functionality. Should just take in a generic DataElement
instance.
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.
Also unit test(s).
|
||
:return: The data file element in the event of a valid element, or None if | ||
it's invalid. | ||
:rtype: DataFileElement | 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.
In isolation, I think it might make more sense if this function returned a boolean.
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.
I agree. I avoided it because compute_many_descriptors
will have to be changed slightly.. but it's no big deal.
:type dfe: DataFileElement | ||
|
||
:return: Whether or not the image is loadable | ||
:rtype bool: |
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.
typo. :rtype bool:
-> :rtype: bool
f52113a
to
40bef39
Compare
@Purg PTAL - My mock-fu isn't up to snuff, I would like to test that the logger is being called with the correct arguments, any suggestions? |
d6133c8
to
5931c3d
Compare
5931c3d
to
5291c9c
Compare
4138aaa
to
00ea90d
Compare
Partially addresses #128 |
For mock testing, I'd say look at this for ideas. Also the mock documentation page of course. |
Fixes #286