SG-12945: Added localized param to Shotgun object #204
Conversation
thebeeland
left a comment
thebeeland
left a comment
There was a problem hiding this comment.
If I'm correct about defaulting localized to true, in that it'll potentially break existing code using this API, then I'd like to see that change to a default of false to maintain the default behavior we have right now going forward. If that doesn't make sense, feel free to reach out.
shotgun_api3/shotgun.py
Outdated
| session_token=None, | ||
| auth_token=None): | ||
| auth_token=None, | ||
| localized=True): |
There was a problem hiding this comment.
I might default this to False. My fear would be that this will change the form of the data returned by the API and might break existing client scripts (or even stuff in Toolkit that we provide) without additional code changes to compensate for it.
|
@palisir -- for testing, would it be possible to use a similar approach to the below to test that the new init argument triggers the header value to be added to the request if it's True, and that it doesn't when it's False? https://github.com/shotgunsoftware/python-api/blob/master/tests/test_client.py#L167-L231 |
|
Is is necessary to pass this in as part of the constructor? In other advanced cases like this (e.g. In fact, there was a similar discussion recently - see #196 - where we ended up adding it to the config object only. |
Reverts included mimetypes module to the unchanged python 2 code and skips some tests to allow CI to pass.
… header when not passing localized param (default)
manneohrstrom
left a comment
manneohrstrom
left a comment
There was a problem hiding this comment.
Hey there!
I think there are some things that need to be looked at before this can be merged:
- I am seeing a lot of unrelated changes in this diff - do you need to merge in master? Alternatively, if these changes are indeed intended to be included in the PR, please update the PR description with more information about them!
- I am not seeing any documentation - please take a look and add docs! If you could include a build of the updated docs look like, that would be great. That's how we normally review these things.
Ping me when you need another round of review and i'll be happy to help!
|
|
||
| Here you can see the full list of changes between each Python API release. | ||
|
|
||
| v3.1.1 (2019 August 29) |
There was a problem hiding this comment.
How does this relate to this particular pull request? I am seeing a bunch of changes in the diff here that seem unrelated to this PR. Do you need to merge with master?
| ``auth_token`` is invalid. | ||
| .. todo: Add this info to the Authentication section of the docs | ||
| :param bool localized: A boolean to asks for some fields to be localized. When ``True``, a |
There was a problem hiding this comment.
This does not match the parameters of the constructor.
| self.session_token = None | ||
| self.authorization = None | ||
| self.no_ssl_validation = False | ||
| self.localized = False |
There was a problem hiding this comment.
i am not seeing any docs or comments to describe what this is. Please add at least a comment - ideally sphinx docs in context of the code, but it's also ok if these are separate and part of the rst file.
|
As discussed with @manneohrstrom I am closing this PR to make a new one on another branch |
The goal of SG-12945 is to expose a new param "localised" on the object Shotgun, to give the ability to get localized fields when possible (only DisplayColumn at the time).
Documentation is on its way too.
As discussed with @thebeeland, the requirement for tests will be evaluated by the reviewer.