Ticket/34463 upload thumbnail

[Zaltu]

No description provided.

[manneohrstrom]

I suggest using in here.

[victoriagrey]

Manne's referring to something along the lines of :
is_thumbnail = (field_name in ["image", "thumb_image", "filmstrip_thumb_image", "filmstrip_image"])

[Zaltu]

Good idea, thanks!

[victoriagrey]

Looks like a solid run at it. I think if you hit Manne's notes this should be good to go.

If you want to be really proactive, you could do a cleanup remove
size = os.stat(path).st_size
from every function except for test_upload_download() :p

[victoriagrey]

So we need the other two lines back in here:

this_dir, _ = os.path.split(__file__)
path = os.path.abspath(os.path.expanduser(
    os.path.join(this_dir,"sg_logo.jpg")))

as they're used to upload the thumbnail image.

the size variable doesn't actually get used further down.

[victoriagrey]

Was this test fixed? It looks like it's still broken...

[Zaltu]

Looks like I commit before saving the file. Oops...

[victoriagrey]

On line 1487, we need to account for the new naming usage as well:
https://github.com/shotgunsoftware/python-api/blob/ticket/34463-upload-thumbnail/shotgun_api3/shotgun.py#L1487

if field_name == "filmstrip_thumb_image":
    params["filmstrip"] = True

Under the new scheme that's not going to get picked up, so the filmstrip will never get uploaded.

@manneohrstrom suggests we should possibly use a class variable to identify these 2 sets of fields (for thumbnails and filmstrips). That way we don't run into things like having to comb the code for other instances of the field name's use.

[Zaltu]

I'll see about making a class variable for these parameters.

[manneohrstrom]

Yep, see how we are using _DATE_TIME_PATTERN for example.

Would be nice to define something like

FILMSTRIP_THUMBNAIL_FIELD = "filmstrip_image"
THUMBNAIL_FIELD = "image"

Which can then be used in the code:

is_thumbnail = field_name in (self.FILMSTRIP_THUMBNAIL_FIELD, self.THUMBNAIL_FIELD)

This is useful for clients of the API as well since they too can use the constant.

Also - do we need to include the legacy fields? Would be nice to quickly check when they were changed over - I have a feeling it was a long time ago so we should check that...

[Zaltu]

After looking at it a bit, I don't think having a class variable would be very useful. The 'image' and 'filmstrip_image' are unique fields and one of the point of changing the code was to not have both old and new versions. It would also be non-standardized as only those two specific fields would be user-accessible as constants. The users should have no more reason to use a constant for those two fields than any other field. Thoughts?

[manneohrstrom]

Hey there Samuel!

I noticed "filmstrip_image" and "image" are hard coded in multiple places. This is typically an indication that a value perhaps should be defined in a single place in the code as a constant rather than being repeated over and over. Your earlier change is in fact case in point ( 031a369 ), you missed updating one place. With a constant, the risk for running into this is reduced.

But this PR has been doing quite a few iterations already, maybe we can skip this change for now and try to get this merged instead? Just to keep things moving? :)

[Zaltu]

Gotcha. I'll go over the next steps tomorrow :)