Python guideline reorg/MQ2020 #2177

johanste · 2020-12-17T23:27:11Z

No description provided.

annatisch

Some preliminary comments :)

docs/python/implementation.md

docs/python/_includes/example_client.py

docs/python/design.md

tjprescott

Thanks for giving me a good excuse to read the Python guidelines in their entirety. Some comments for consideration.

docs/python/design.md

docs/python/implementation.md

annatisch · 2021-01-25T01:15:24Z

Working on some updates and feedback here:
johanste#1

Some updates from feedback

annelo-msft

This is awesome; left some comments. Thanks, Johan!!

docs/python/design.md

docs/python/implementation.md

Co-authored-by: Anne Thompson <annelo@microsoft.com>

xiangyan99 · 2021-02-03T02:26:31Z

Could you add some guideline for the scenario

def _do_something(input):
    do_something

class Client():
    def method(self):
        _do_something()

vs

class Client():
    @classmethod
    def _do_something(cls, input):
         do_something

    def method(self):
         cls._do_something()

vs

class Client():
    @staticmethod
    def _do_something(input):
         do_something

    def method(self):
         Client._do_something()

annatisch · 2021-02-08T03:22:46Z

docs/python/design.md


-TODO: Add an example of the basic API shape similar to what's shown in the code sample <!--[here](https://azure.github.io/azure-sdk/java_design.html#java-sync-client-shape)--> but with the equivalent in Python.  Please include code illustrations of the following areas:
+{% include requirement/MUST id="python-general-version-support" %} support Python 2.7 and 3.5.3+.


Should this now be upgraded to 3.6+?

We need to announce 3.5 deprecation first. Which we probably have to very soon given that other dependencies are also giving up on it.

docs/python/design.md

docs/python/implementation.md

docs/python/design.md

docs/python/implementation.md

tjprescott · 2021-02-16T18:25:56Z

docs/python/implementation.md

+{% include requirement/MUST id="python-codestyle-pep484" %} provide type hints [PEP484](https://www.python.org/dev/peps/pep-0484/) for publicly documented classes and functions.
+
+- See the [suggested syntax for Python 2.7 and 2.7-3.x straddling code](https://www.python.org/dev/peps/pep-0484/#suggested-syntax-for-python-2-7-and-straddling-code) for guidance for Python 2.7 compatible code.
+


Does this mean all code should use the 2.7 syntax, or should 3.x code (like async clients) use the 3.x compatible syntax??

Python 3 only code should use Python 3 syntax.

Might be worth calling out since some might just apply the 2.7 syntax everywhere

Called out. Hopefully I'll be able to clean up all references on "how to do X for Python 2.7 support) soon. Very soon. Any day now.

tjprescott

Added guidance on positional vs. kword-only would help.

tjprescott · 2021-02-16T18:37:07Z

docs/python/implementation.md

+See TODO: insert link for general guidance on positional vs. optional parameters here.

-{% include requirement/MUST id="python-tooling-flake8" %} use [flake8-docstrings](https://gitlab.com/pycqa/flake8-docstrings) to verify doc comments.
+{% include requirement/MUST id="python-codestyle-optional-args" %} use keyword-only arguments for optional or less-often-used arguments for modules that only need to support Python 3.

-{% include requirement/MUST id="python-tooling-black" %} use [Black](https://black.readthedocs.io/en/stable/) for formatting your code.
+```python
+# Yes
+def foo(a, b, *, c, d=None):
+    # Note that I can even have required keyword-only arguments...
+    ...
+```

-{% include requirement/SHOULD id="python-tooling-mypy" %} use [MyPy](https://mypy.readthedocs.io/en/latest/) to statically check the public surface area of your library.
+{% include requirement/MUST id="python-codestyle-kwargs" %} use keyword-only arguments for arguments that have no obvious ordering.


It would be great if this TODO could be resolved in this PR. Namely these two guidelines seem at odds.

Here's the scenario that came up where these guidelines caused some friction:

def __init__(self, remote_rendering_endpoint, account_id, account_domain, credential, **kwargs)

All of the positional arguments are required, but the arguments have no obvious ordering. Yet, in practice, we would recommend these all be kept positional vice kwarg-only.

It doesn't add clarity that example for the kwarg-only for optional args guidelines even says:

# Yes def foo(a, b, *, c, d=None): # Note that I can even have required keyword-only arguments... ...

cc/ @annatisch @rikogeln

In practical terms, we can have required **kwargs as well. It's just that you have to do a runtime check. Since the intent is to - in many cases - emulate keyword-only arguments in our use of kwargs in Python 2 compatible code, I'll add this to the guidance.

On a related note, I've opened Azure/azure-sdk-for-python#16772 to track guidance on overloads since that is tangentially related to this...

kristapratico · 2021-02-06T01:48:11Z

docs/python/_includes/example_client.py

+        :type endpoint str:
+        :param credential: Credentials to use when connecting to the service.
+        :type credential: ~azure.core.credentials.TokenCredential
+        :keyword apiversion: API version to use when talking to the service. Default is '2020-12-31'


small nit: apiversion in docstring doesn't match api_version naming below

kristapratico · 2021-02-06T02:06:46Z

docs/python/design.md

+|`timeout`|Timeout in seconds|All service methods|
+|`headers`|Custom headers to include in the service request|All requests|Headers are added to all requests made (directly or indirectly) by the method.|
+|`client_request_id`|Caller specified identification of the request.|Service operations for services that allow the client to send a client-generated correlation ID.|Examples of this include `x-ms-client-request-id` headers.|The client library **must** use this value if provided, or generate a unique value for each request when not specified.|
+|`response_hook`|`callable` that is called with (response, headers) for each operation.|All service methods|


kristapratico · 2021-02-18T02:43:25Z

docs/python/design.md

+
+##### Specifying the Service Version
+
+{% include requirement/MUST id="python-client-constructor-api-version-argument-1" %} accept an optional `api_version` keyword-only argument of type string. If specified, the provided api version MUST be used when interacting with the service. If the parameter is not provided, the default value MUST be the latest non-preview API version understood by the client library (if there the service has a non-preview version) or the latest preview API version understood by the client library (if the service does not have any non-preview API versions yet). This parameter MUST be available even if there is only one API version understood by the service in order to allow library developers to lock down the API version they expect to interact with the service with.


Suggested change

{% include requirement/MUST id="python-client-constructor-api-version-argument-1" %} accept an optional `api_version` keyword-only argument of type string. If specified, the provided api version MUST be used when interacting with the service. If the parameter is not provided, the default value MUST be the latest non-preview API version understood by the client library (if there the service has a non-preview version) or the latest preview API version understood by the client library (if the service does not have any non-preview API versions yet). This parameter MUST be available even if there is only one API version understood by the service in order to allow library developers to lock down the API version they expect to interact with the service with.

{% include requirement/MUST id="python-client-constructor-api-version-argument-1" %} accept an optional `api_version` keyword-only argument of type string. If specified, the provided api version MUST be used when interacting with the service. If the parameter is not provided, the default value MUST be the latest non-preview API version understood by the client library (if the service has a non-preview version) or the latest preview API version understood by the client library (if the service does not have any non-preview API versions yet). This parameter MUST be available even if there is only one API version understood by the service in order to allow library developers to lock down the API version they expect to interact with the service with.

kristapratico · 2021-02-18T02:45:14Z

docs/python/design.md

+
+{% include requirement/MAY id="python-client-constructor-api-version-argument-4" %} validate the input `api_version` value against a list of supported API versions.
+
+{% include requirement/MAY id="python-client-constructor-api-version-argument-5" %} include all service API versions that are supported by the client library in a `ServiceVersion` enumerated value.


Should this be <Service>Version? Like TextAnalyticsVersion?

kristapratico · 2021-02-18T02:48:01Z

docs/python/design.md

+    exists = client.resource_exists(name):
+    if not exists:
+        print("The resource doesn't exist...")
+except azure.core.errors.ServiceRequestError:


nit: should be azure.core.exceptions

kristapratico · 2021-02-18T03:08:23Z

docs/python/design.md


-TODO: If there are design considerations to call out that parallel the xxOptions parameters, please add those here.  e.g. per the Python API Design Training Article Anna put together at **github.com/Azure/azure-sdk-pr/blob/master/training/azure-sdk-apis/getting-started/design-the-api/design-the-api-python.md#the-get-request**, what should be positional parameters vs. kwargs?
+{% include requirement/MUST id="python-verioning-minor" %} increment the minor version if any new functionality is added to the package.


Suggested change

{% include requirement/MUST id="python-verioning-minor" %} increment the minor version if any new functionality is added to the package.

{% include requirement/MUST id="python-versioning-minor" %} increment the minor version if any new functionality is added to the package.

kristapratico · 2021-02-18T03:09:42Z

docs/python/design.md


-{% include requirement/MUST id="python-response-logical-entity" %} optimize for returning the logical entity for a given request. The logical entity MUST represent the information needed in the 99%+ case.
+{% include requirement/MUST id="python-versioning-major-cross-languages" %} select a version number greater than the highest version number of any other released Track 1 package for the service in any other scope or language.


I thought the rule we were going by was:

The maximum released version of a Track 1 library across all languages +1

kristapratico · 2021-02-18T03:13:00Z

docs/python/design.md

+
+{% include requirement/MUST id="python-samples-runnable" %} ensure that each sample file is runnable.
+
+{% include requirement/MUST id="python-samples-coding-style" %} use Python 3 conventions when creating samples. Do not include Python 2 compatibility code (e.g. using [`six`](https://six.readthedocs.io)) since that will distract from what you are trying to present. Avoid using features newer than the Python 3 baseline support. The current supported Python version is 3.6.


The current supported Python version is 3.6.

I thought we supported 3.5?

kristapratico · 2021-02-18T03:19:45Z

docs/python/implementation.md

    continuation_token: "str"

    def by_page(self) -> ByPagePaged[T] ...
 ```

-`azure.core.Paged` implements the `Paged` protocol.
+`azure.core.ItemPaged` implements the `ItemPaged` protocol.


nit: azure.core.paging.ItemPaged

docs/python/implementation.md

Initial draft of guideline reorg/MQ2020

653d3e3

johanste requested a review from annatisch December 17, 2020 23:27

johanste added 2 commits December 17, 2020 15:28

Merge branch 'master' into dec2020mq

f878910

Fixed half the todos

fcfaae8

annatisch reviewed Jan 5, 2021

View reviewed changes

johanste changed the title ~~Initial draft of guideline reorg/MQ2020~~ Initial draft of python guideline reorg/MQ2020 Jan 6, 2021

johanste added 4 commits January 6, 2021 14:42

Addressed review feedback, improved example.

7a7f1ec

Python: moved intro into design, restructured

383adcc

Added guidance on cancellation

45d33c7

Added guidance on client immutability

d450569

tjprescott reviewed Jan 22, 2021

View reviewed changes

Some updates from feedback

0cc5299

johanste added 6 commits January 25, 2021 20:24

python: fix broken link in refs.md

c9d9a8b

Merge pull request #1 from annatisch/anna-dec2020mq

e541565

Some updates from feedback

Fix typo in python.md directive

6e4de86

Python: added documentation section

ed0e642

Python: removed serialization implementation section

4aa0229

Python: fixed up title levels

0999e40

johanste changed the title ~~Initial draft of python guideline reorg/MQ2020~~ Python guideline reorg/MQ2020 Feb 1, 2021

johanste marked this pull request as ready for review February 1, 2021 01:02

johanste requested review from annelo-msft, czubair, kyle-patterson, salameer and weshaggard as code owners February 1, 2021 01:02

johanste requested review from bterlson, KrzysztofCwalina, JeffreyRichter and JonathanGiles February 1, 2021 06:34

annelo-msft approved these changes Feb 1, 2021

View reviewed changes

johanste and others added 3 commits February 2, 2021 13:39

Update docs/python/design.md

378b830

Co-authored-by: Anne Thompson <annelo@microsoft.com>

Update docs/python/design.md

e03557e

Co-authored-by: Anne Thompson <annelo@microsoft.com>

Update docs/python/implementation.md

1fdb143

Co-authored-by: Anne Thompson <annelo@microsoft.com>

johanste added 2 commits February 3, 2021 13:55

Python guidelines: review feedback

24550d2

Merge branch 'dec2020mq' of github.com:johanste/azure-sdk into dec2020mq

b81bb1a