feat: python rtl optional config ApiSettings init parameter #996

kevin-wallace-wpro · 2022-02-18T18:47:53Z

Supersedes #972

This PR adds an optional config_settings parameter to the python sdk init functions. It is up to the sdk user to provide a valid implementation of the ApiSettings class.

Ensure the tests and linter pass
Appropriate docs were updated (if necessary)

joeldodge79

Fantastic! besides my suggested readme change I have one more request to update this code

ideally some typing foo that specifies the return dict must have client_id: str and client_secret: str keys but may have any other keys as well
if that's not easily available then at least comment (or docstring?) on that method declaration indicating that those keys are necessary. PEP665 would have been good but we ned a solution that supports at least 3.7 (and ideally 3.6)

joeldodge79 · 2022-02-22T15:37:20Z

python/README.rst

@@ -132,6 +137,35 @@ example file:
 For any ``.ini`` setting you can use an environment variable instead. It takes the form of
 ``LOOKERSDK_<UPPERCASE-SETTING-FROM-INI>`` e.g. ``LOOKERSDK_CLIENT_SECRET``

+A final option is to provide your own implementation of the ApiSettings class. In particular you may want to override the ``read_config`` function. Example:


Suggested change

A final option is to provide your own implementation of the ApiSettings class. In particular you may want to override the ``read_config`` function. Example:

A final option is to provide your own implementation of the ApiSettings class. It is easiest to subclass ``api_settings.ApiSettings`` and override ``read_config`` function (don't forget a call to ``super().read_config()`` if appropriate, Example below). However, at a minimum your class must implement the `api_settings.PApiSettings` protocol

python/README.rst

kevin-wallace-wpro

Updated comments and added type for read_config

kevin-wallace-wpro · 2022-02-22T22:31:48Z

python/looker_sdk/rtl/api_settings.py

+from typing_extensions import Required
+
+class SettingsConfig(TypedDict, total=False):
+    client_id: Required[str]


Required is a relatively recent addition to typing_extensions, looks like about 3 months ago.

pyright support is there which is nice..

cool! I'm approving action runs so we can see how this goes on all the python versions we support

kevin-wallace-wpro · 2022-02-22T22:31:51Z

python/looker_sdk/rtl/api_settings.py


 class PApiSettings(transport.PTransportSettings, Protocol):
-    def read_config(self) -> Dict[str, str]:
+    def read_config(self) -> SettingsConfig:


I've added the type here and my editor was happy with it. Adding it to read_config would probably require further code changes, probably not worth it?

let's see how the mypy and unittest runs go. If TypeDict.Required is available on all our supported python versions then it actually would be nice (actually probably required) to update the default implementation's type signature

joeldodge79

You'll need to sign the Google CLA to get this in (see the google/cla check that's failing and click details for instructions on how to sign it)

joeldodge79 · 2022-02-23T02:08:14Z

python/looker_sdk/rtl/api_settings.py

+from typing_extensions import Required
+
+class SettingsConfig(TypedDict, total=False):
+    client_id: Required[str]


cool! I'm approving action runs so we can see how this goes on all the python versions we support

joeldodge79 · 2022-02-23T02:10:21Z

python/looker_sdk/rtl/api_settings.py

+class SettingsConfig(TypedDict, total=False):
+    client_id: Required[str]
+    client_secret: Required[str]
+    base_url: Required[str]


base_url is technically not required. It's required to be an instance property on the settings object and the default implementation gets it from read_config but that's not required. I know it's confusing - honestly the only reason read_config exists is so that our default implementation doesn't have to store client_id or client_secret in python memory for security purposes. Otherwise they'd just be required instance properties too and there would be no read_config

joeldodge79 · 2022-02-23T02:11:43Z

python/looker_sdk/rtl/api_settings.py


 class PApiSettings(transport.PTransportSettings, Protocol):
-    def read_config(self) -> Dict[str, str]:
+    def read_config(self) -> SettingsConfig:


let's see how the mypy and unittest runs go. If TypeDict.Required is available on all our supported python versions then it actually would be nice (actually probably required) to update the default implementation's type signature

joeldodge79 · 2022-02-23T02:14:34Z

yup, mypy run failed with looker_sdk/rtl/api_settings.py:104: error: Return type "Dict[str, str]" of "read_config" incompatible with return type "SettingsConfig" in supertype "PApiSettings" before we could run tox tests on other python versions. I'll do a quick experiment to see if that works and is worth pursuing the Required stuff

joeldodge79 · 2022-02-23T02:23:08Z

If #1005 passes CI then I think it would be great if you could fix all the mypy errors so that we can use def read_config(self) -> SettingsConfig

joeldodge79 · 2022-02-23T14:35:49Z

python/looker_sdk/rtl/api_settings.py

 if sys.version_info >= (3, 8):
-    from typing import Protocol
+    from typing import Protocol, TypedDict
 else:
-    from typing_extensions import Protocol
+    from typing_extensions import Protocol, TypedDict

+from typing_extensions import Required


Now that we're using typing_extensions for Required and it's not available till 3.11(?) we need to update this and setup.py (and we should specify the minimum version of typing_extensions to use)

if sys.version_info >= (3, 8): from typing import Protocol else: from typing_extensions import Protocol, TypedDict if sys.version_info >= (3, 11): from typing import Required else: from typing_extensions import Required

starting to get a little unwieldy - I've seen other libraries consolidate into a _compat.py library to do all this. But for now I think we're ok leaving it inline here.

I tried with 3.11.0a4 but Required wasn't in there.

So I've

kept the import coming from typing_extensions

updated setup.py to require typing-extensions >= 4.1.1 regardless of the python version

I think that is the best way? Note we still can't use 3.11 anyway because of #944 (I pulled Required from typing_extensions running 3.11 and got the error referenced in that issue).

joeldodge79 · 2022-02-23T14:36:56Z

If #1005 passes CI

failed with

tests/conftest.py:8: in <module>
    import looker_sdk  # noqa: E402
looker_sdk/__init__.py:[25](https://github.com/looker-open-source/sdk-codegen/runs/5297804600?check_suite_focus=true#step:5:25): in <module>
    from looker_sdk.rtl import api_settings
looker_sdk/rtl/api_settings.py:40: in <module>
    from typing_extensions import Required
E   ModuleNotFoundError: No module named 'typing_extensions'

provided some relevant guidance inline I appreciate your effort on this!

kevin-wallace-wpro · 2022-02-24T01:49:34Z

python/looker_sdk/rtl/api_settings.py

@@ -122,6 +135,21 @@ def _bool(val: str) -> bool:
            raise TypeError
        return converted

+    def _override_settings(


We can avoid this with some casts, but I couldn't get anything else to infer the correct types without explicitly using the SettingsConfig keys.

hm actually I see now there is some use of cast elsewhere. Personally I like the cast solution better, less code changes and personally I feel the code is more future-proof, this way we could easily miss keys we want.

I'll put up a version with that so you can compare

kevin-wallace-wpro · 2022-02-24T01:52:31Z

python/looker_sdk/rtl/api_settings.py

        cfg_parser = cp.ConfigParser()
+        data: SettingsConfig = {


If the user doesn't end up settings these they'll run into this exception

joeldodge79

Looking good!

I don't know why I always have to approve your check runs. thought that was a one-time thing. anyway, getting closer but mypy still failed:

looker_sdk/rtl/auth_session.py:276: error: Expression has type "Any"
looker_sdk/rtl/auth_session.py:276: error: TypedDict "SettingsConfig" has no key "redirect_uri"
looker_sdk/rtl/auth_session.py:277: error: Expression has type "Any"
looker_sdk/rtl/auth_session.py:277: error: TypedDict "SettingsConfig" has no key "looker_url"
looker_sdk/rtl/auth_session.py:286: error: Expression type contains "Any" (has type "Tuple[str, Any]")
looker_sdk/rtl/auth_session.py:286: error: Expression has type "Any"
looker_sdk/rtl/auth_session.py:292: error: Expression has type "Any"
looker_sdk/rtl/auth_session.py:294: error: Expression has type "Any"
looker_sdk/rtl/auth_session.py:338: error: Expression has type "Any"
looker_sdk/rtl/auth_session.py:349: error: Expression has type "Any"

points out some inconsistencies we have - added "OAuthSession" support that needs redirect_uri and looker_url so we should add those to both SettingConfig and PApiSettings as optional.

you should be able to check it locally (might need to install requests-types) with:

pipenv run mypy looker_sdk/

joeldodge79 · 2022-02-24T02:37:08Z

python/looker_sdk/rtl/api_settings.py

@@ -122,6 +135,21 @@ def _bool(val: str) -> bool:
            raise TypeError
        return converted

+    def _override_settings(


kevin-wallace-wpro · 2022-02-24T05:57:50Z

points out some inconsistencies we have - added "OAuthSession" support that needs redirect_uri and looker_url so we should add those to both SettingConfig and PApiSettings as optional.

Ah whoops ok I'll take a look at it. I was actually running mypy locally but there were quite a few complaints about Any so I just figured these were already there and acceptable without much thought haha.

kevin-wallace-wpro

Ok added redirect_uri and looker_url to SettingsConfig. Simplified _override_settings to just loop over SettingsConfig's keys (we could do the same thing in _override_from_env if you want?). Reverted _clean_input to original impl.

kevin-wallace-wpro · 2022-02-25T00:34:34Z

python/looker_sdk/rtl/api_settings.py

@@ -146,7 +170,7 @@ def _override_from_env(self) -> Dict[str, str]:

        return overrides

-    def _clean_input(self, data: Dict[str, str]) -> Dict[str, str]:
+    def _clean_input(self, data: SettingsConfig) -> SettingsConfig:


Reverted back to more or less the original version here with a cast at the end.

works for me!

joeldodge79

Nice work! really appreciate it.

mypy and unittests are passing. our integration tests won't run/pass because they use a secret that doesn't transfer to forked repos. So I'm gonna update this branch and then override the failed integration test checks and merge it in. If, for some reason, our integration tests fail I'll forward fix them

joeldodge79 · 2022-02-25T00:55:13Z

python/looker_sdk/rtl/api_settings.py

@@ -146,7 +170,7 @@ def _override_from_env(self) -> Dict[str, str]:

        return overrides

-    def _clean_input(self, data: Dict[str, str]) -> Dict[str, str]:
+    def _clean_input(self, data: SettingsConfig) -> SettingsConfig:


works for me!

feat, python rtl: optional config ApiSettings init parameter

79017c3

kevin-wallace-wpro mentioned this pull request Feb 18, 2022

feat(python rtl): add optional config dictionary init parameter #972

Closed

2 tasks

joeldodge79 suggested changes Feb 22, 2022

View reviewed changes

joeldodge79 reviewed Feb 22, 2022

View reviewed changes

python/README.rst Show resolved Hide resolved

add SettingsConfig TypedDict, update comments

348fa80

kevin-wallace-wpro commented Feb 22, 2022

View reviewed changes

joeldodge79 reviewed Feb 23, 2022

View reviewed changes

kevin-wallace-wpro added 2 commits February 23, 2022 17:32

update read_config type to return SettingsConfig

354f78d

remove unused type

a004173

kevin-wallace-wpro commented Feb 24, 2022

View reviewed changes

joeldodge79 suggested changes Feb 24, 2022

View reviewed changes

loop over keys, ignore mypy

ae33294

kevin-wallace-wpro commented Feb 25, 2022

View reviewed changes

joeldodge79 changed the title ~~feat, python rtl: optional config ApiSettings init parameter~~ feat: python rtl optional config ApiSettings init parameter Feb 25, 2022

joeldodge79 approved these changes Feb 25, 2022

View reviewed changes

Merge branch 'main' into init-via-user-provided-settings

3e24d6a

joeldodge79 merged commit 3ae42cd into looker-open-source:main Feb 25, 2022

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: python rtl optional config ApiSettings init parameter #996

feat: python rtl optional config ApiSettings init parameter #996

kevin-wallace-wpro commented Feb 18, 2022 •

edited

Loading

joeldodge79 left a comment

joeldodge79 Feb 22, 2022

kevin-wallace-wpro left a comment

kevin-wallace-wpro Feb 22, 2022

joeldodge79 Feb 23, 2022

kevin-wallace-wpro Feb 22, 2022

joeldodge79 Feb 23, 2022

joeldodge79 left a comment

joeldodge79 Feb 23, 2022

joeldodge79 Feb 23, 2022

joeldodge79 Feb 23, 2022

joeldodge79 commented Feb 23, 2022

joeldodge79 commented Feb 23, 2022

joeldodge79 Feb 23, 2022

kevin-wallace-wpro Feb 24, 2022

joeldodge79 commented Feb 23, 2022

kevin-wallace-wpro Feb 24, 2022

joeldodge79 Feb 24, 2022

kevin-wallace-wpro Feb 24, 2022

kevin-wallace-wpro Feb 24, 2022

joeldodge79 left a comment

joeldodge79 Feb 24, 2022

kevin-wallace-wpro commented Feb 24, 2022

kevin-wallace-wpro left a comment

kevin-wallace-wpro Feb 25, 2022

joeldodge79 Feb 25, 2022

joeldodge79 left a comment

joeldodge79 Feb 25, 2022

This comment has been minimized.

	A final option is to provide your own implementation of the ApiSettings class. In particular you may want to override the ``read_config`` function. Example:
	A final option is to provide your own implementation of the ApiSettings class. It is easiest to subclass ``api_settings.ApiSettings`` and override ``read_config`` function (don't forget a call to ``super().read_config()`` if appropriate, Example below). However, at a minimum your class must implement the `api_settings.PApiSettings` protocol

feat: python rtl optional config ApiSettings init parameter #996

feat: python rtl optional config ApiSettings init parameter #996

Conversation

kevin-wallace-wpro commented Feb 18, 2022 • edited Loading

joeldodge79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-wallace-wpro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 commented Feb 23, 2022

joeldodge79 commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-wallace-wpro commented Feb 24, 2022

kevin-wallace-wpro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

kevin-wallace-wpro commented Feb 18, 2022 •

edited

Loading