-
Notifications
You must be signed in to change notification settings - Fork 49
Colorspace now allows empty file representation list #1210
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
base: develop
Are you sure you want to change the base?
Conversation
filename = representation["files"] | ||
if isinstance(filename, list): | ||
filename = filename[0] | ||
filename = representation["files"][0] if representation["files"] else 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.
This logic is wrong unfortunately. files
may be a single filename str
... so we'd still need the list
check at least.
filename = representation["files"][0] if representation["files"] else None | |
filename = representation["files"] | |
if isinstance(filename, list): | |
if filename: | |
filename = filename[0] | |
else: | |
filename = None |
However, I really don't think this is the best fix here. We're suddenly allowing this to continue in a way where it may not be sensible to actually continue. I think this erroring is correct and hence we should handle the error elsewhere instead.
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.
My intention is just to pass the error to validator that is working properly, with message that artist can understand.
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 intention is good I think - but I think it makes more sense for collect_writes.py
to NOT set representation colorspace IF there are no files if apparently, it cannot? Or at least capture it accordingly instead of silently ignoring it here where it we're expecting it to just work... instead of ignore silently?
Co-authored-by: Roy Nieterau <[email protected]>
if isinstance(filename, list): | ||
filename = filename[0] | ||
if filename: | ||
filename = filename[0] | ||
else: | ||
filename = 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.
The filename
is needed only if colorspace is None
so we should move it under the condition. And we should not set it to None
as that would cause crash in get_imageio_file_rules_colorspace_from_filepath
.
Changelog Description
Colorspace.py now check for empty file representation list.
Testing notes:
Can fix #97
Might also fix #66