SG-12945 + SG-12581 : Added localized param to Shotgun object #208

palisir · 2019-09-05T17:51:06Z

The goal of SG-12945 is to expose a new config property named "localized" on the object Shotgun, to give the ability to get localized display names from the server when possible.

The code is tested and documented (according to SG-12581).

…ault)

coveralls · 2019-09-05T20:43:54Z

Pull Request Test Coverage Report for Build 1234

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.06%) to 69.552%

Files with Coverage Reduction	New Missed Lines	%
shotgun_api3/shotgun.py	2	82.77%

Totals
Change from base Build 1207:	-0.06%
Covered Lines:	1366
Relevant Lines:	1964

💛 - Coveralls

manneohrstrom · 2019-09-06T11:36:57Z

This is looking awesome! Thank you, @palisir! For reference, the built docs look like this:

manneohrstrom

This looks great! Loving the docs! Awesome job! Thank you for the new PR and the lovely wrapup!
See comments - mainly around formatting of the docs! No need for further review from me (unless you want to)!

I will add @willis102 as an additional reviewer just to take a quick look at thg python 3 side of things - to ensure that the approach of returning the localized data makes sense in python3 as well as python2.

manneohrstrom · 2019-09-06T11:38:08Z

docs/reference.rst

+Localization
+************
+
+Shotgun API offers the ability to return display namnes in user's language.


The Shotgun API

How does the API know what the user's language is? I think this is worth explaining. Is it based on the user's language preferences in Shotgun? (What happens with script/API users?)

namnes (typo)

suggestion: return display namnes in user's language -> return field display names in the current user's language.

Thanks for all the suggestions ! I updated the documentation.

manneohrstrom · 2019-09-06T11:45:31Z

docs/reference.rst

+  * Shotgun.schema_field_read
+
+Localization is disabled by default. To enable localization, set the ``localized`` property to ``True``.
+The property cannot be set at Shotgun class instantiation. When the class is instantiated, the ``localized`` config property is set to ``False``.


line 905: i think we can skip this entire line! The example explains it well!

manneohrstrom · 2019-09-06T11:45:53Z

docs/reference.rst

+Localization is disabled by default. To enable localization, set the ``localized`` property to ``True``.
+The property cannot be set at Shotgun class instantiation. When the class is instantiated, the ``localized`` config property is set to ``False``.
+
+Example for a user whose Language preference is set to Japanese:


nitpick: language should have a lower case l

manneohrstrom · 2019-09-06T11:46:04Z

docs/reference.rst

+Example for a user whose Language preference is set to Japanese:
+
+.. code-block:: python
+   :emphasize-lines: 8,19


manneohrstrom · 2019-09-06T11:47:19Z

docs/reference.rst

+    'sg_vendor_groups': {
+        'mandatory': {'editable': False, 'value': False},
+        # the value field (display name) is localized
+        'name': {'editable': True, 'value': '\xe3\x83\x99\xe3\x83\xb3\xe3\x83\x80\xe3\x83\xbc \xe3\x82\xb0\xe3\x83\xab\xe3\x83\xbc\xe3\x83\x97'},


this is a UTF-8 encoded string, right? I think we should mention that as part of the docs.

Also, it would be great if @willis102 did a quick CR on this to make sure that this approach still make sense for Python 3!

So yes, this is returning a UTF-8 encoded string in Python 2, and a str in Python 3 (meaning in Python 3 we're not dealing with bytes.) It might be good to mention in the documentation that for Python 2/3 compatibility you can decode the string using shotgun_api3.lib.six.ensure_text(), ensuring that the Python3 str object is handled properly. I ran your test code in the branch on Python 2 and 3 and it behaved as I would expect.

So, my opinion is that this would be good to mention in the docs but that the current behavior is correct.

manneohrstrom · 2019-09-06T11:52:19Z

docs/reference.rst

+************
+
+Shotgun API offers the ability to return display namnes in user's language.
+The server currently returns localized display names for those methods:


I suggest changing this line (and removing the two bullet points below) to "This functionality is currently supported by the methods schema_entity_read and schema_field_read."

manneohrstrom · 2019-09-06T11:55:22Z

shotgun_api3/shotgun.py

        self.config.convert_datetimes_to_utc = convert_datetimes_to_utc
        self.config.no_ssl_validation = NO_SSL_VALIDATION
        self.config.raw_http_proxy = http_proxy
+        self.config.localized = False


i don't think we need this line - it's already initialized in the constructor for _Config, right? If we do need it, please explain the reason why with a comment!

That's correct, I removed the line

manneohrstrom · 2019-09-06T11:55:34Z

shotgun_api3/shotgun.py

        :rtype: dict
+
+        .. note::
+            The returned display names for this method will be localized when the ``localize`` Shotgun config property is set to ``True``. See :ref:`localization` for more informations.


nitpick: typo: informations -> information.

manneohrstrom · 2019-09-06T11:56:18Z

shotgun_api3/shotgun.py

            If you don't specify a ``project_entity``, everything is reported as visible.

+        .. note::
+            The returned display names for this method will be localized when the ``localize`` Shotgun config property is set to ``True``. See :ref:`localization` for more informations.


nitpick: typo: informations -> information.

manneohrstrom · 2019-09-06T11:57:47Z

tests/test_client.py

+        (_, _, _, headers) = args
+        expected_header_value = "auto"
+
+        self.assertEqual("auto", headers.get("locale"))


I think we probably should test whether the schema content coming back from the server is actually localised. Is this easy to do?

It's not hard to do (not sure if we can set the site language from the API, though) but then it would test the server, not the client.
IMO if the requests is made with the right header, the job is done.
Also, the localization on the server is already tested with Cypress on top of Ruby unit tests.

that makes sense! I was thinking about testing the "receiving end" of the transport, eg. asserting that the received object is a str and that it's utf-8 encoded etc. Maybe this is one for @willis102 to weigh in on - do you think this test would be meaningful in a py3 cpntext?

If I understand correctly, since the encoding/decoding of received objects returned from _call_rpc is tested elsewhere, so we should be covered by those tests already for this as well. If we do add a test for this, it's important to note that on py3 we will get back a str object, not bytes, so it will not need to be decoded as in py2. As mentioned above, six.ensure_text() will ensure that the value is decoded properly on py2 and 3.

…rmation

willis102 · 2019-09-06T19:40:49Z

docs/reference.rst

+    'sg_vendor_groups': {
+        'mandatory': {'editable': False, 'value': False},
+        # the value field (display name) is localized
+        'name': {'editable': True, 'value': '\xe3\x83\x99\xe3\x83\xb3\xe3\x83\x80\xe3\x83\xbc \xe3\x82\xb0\xe3\x83\xab\xe3\x83\xbc\xe3\x83\x97'},


So yes, this is returning a UTF-8 encoded string in Python 2, and a str in Python 3 (meaning in Python 3 we're not dealing with bytes.) It might be good to mention in the documentation that for Python 2/3 compatibility you can decode the string using shotgun_api3.lib.six.ensure_text(), ensuring that the Python3 str object is handled properly. I ran your test code in the branch on Python 2 and 3 and it behaved as I would expect.

So, my opinion is that this would be good to mention in the docs but that the current behavior is correct.

willis102 · 2019-09-06T19:52:27Z

tests/test_client.py

+        (_, _, _, headers) = args
+        expected_header_value = "auto"
+
+        self.assertEqual("auto", headers.get("locale"))


If I understand correctly, since the encoding/decoding of received objects returned from _call_rpc is tested elsewhere, so we should be covered by those tests already for this as well. If we do add a test for this, it's important to note that on py3 we will get back a str object, not bytes, so it will not need to be decoded as in py2. As mentioned above, six.ensure_text() will ensure that the value is decoded properly on py2 and 3.

…ization

Paul ALISIR and others added 7 commits September 4, 2019 11:55

SG-12945: Added localized param to Shotgun object

1c0014c

set default localization param to False

151afde

added test for locale header

1cfd074

added instruction for good syntax on example config file

16744ca

removed localized param from constructor

f550358

added test for localized header when not passing localized param (def…

756cfcb

…ault)

added documentation for localization

c695d7f

palisir requested a review from manneohrstrom September 5, 2019 17:52

replaced default property after removing param from constructor

e44943f

manneohrstrom approved these changes Sep 6, 2019

View reviewed changes

manneohrstrom requested a review from willis102 September 6, 2019 12:00

palisir added 3 commits September 6, 2019 10:26

updated documentation to fix various typos/mistakes and add more info…

f51d242

…rmation

fixed more doc and removed double property assignment

c5b3647

Removed redundant information from documentation

d8f3443

palisir self-assigned this Sep 6, 2019

willis102 approved these changes Sep 6, 2019

View reviewed changes

willis102 self-requested a review September 6, 2019 19:38

willis102 approved these changes Sep 6, 2019

View reviewed changes

palisir added 4 commits September 9, 2019 16:09

updated highlighted lines in doc localization code example

7f37165

added note on localized string decoding in the doc

a688c7a

added Shotgun.schema_read() to the list of methods supported by local…

053ccb2

…ization

packaging for the v3.1.2 release

53f98ca

palisir merged commit 0988a4a into master Sep 17, 2019

000paradox000 deleted the SG-12945_add_localized_param branch September 19, 2021 14:20

SG-12945 + SG-12581 : Added localized param to Shotgun object #208

SG-12945 + SG-12581 : Added localized param to Shotgun object #208

Uh oh!

Conversation

palisir commented Sep 5, 2019

Uh oh!

coveralls commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1234

💛 - Coveralls

Uh oh!

manneohrstrom commented Sep 6, 2019

Uh oh!

manneohrstrom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Sep 5, 2019 •

edited

Loading