MousaZeidBaker / aws-lambda-typing

Python type hints for AWS Lambda
MIT License
106 stars 19 forks source link

Adding CloudFormationCustomResourceEvents #22

Closed tekdj7 closed 2 years ago

tekdj7 commented 2 years ago

Adding CloudFormationEvent and missing exports

tekdj7 commented 2 years ago

also, you need to add pytest as a --dev dependency

MousaZeidBaker commented 2 years ago

also, you need to add pytest as a --dev dependency

pytest is not used, its a typo introduced by last PR will update it to mypy which is already a dev dependency

MousaZeidBaker commented 2 years ago

__all__ ONLY affects the from <module> import * behavior.

From the python docs:

The import statement uses the following convention: if a package’s init.py code defines a list named all, it is taken to be the list of module names that should be imported when from package import * is encountered. It is up to the package author to keep this list up-to-date when a new version of the package is released. *Package authors may also decide not to support it, if they don’t see a use for importing from their package**.

From pep8:

Modules that are designed for use via from M import * should use the __all__ mechanism to prevent exporting globals, or use the older convention of prefixing such globals with an underscore (which you might want to do to indicate these globals are "module non-public").

This package is not designed to be used via from <module> import * thus I think we don't need the __all__ mechanism. Also we will need to keep the this list up-to-date, and since it doesn't add any value I suggest we don't use it.

tekdj7 commented 2 years ago

__all__ ONLY affects the from <module> import * behavior.

From the python docs:

The import statement uses the following convention: if a package’s init.py code defines a list named all, it is taken to be the list of module names that should be imported when from package import * is encountered. It is up to the package author to keep this list up-to-date when a new version of the package is released. *Package authors may also decide not to support it, if they don’t see a use for importing from their package**.

From pep8:

Modules that are designed for use via from M import * should use the __all__ mechanism to prevent exporting globals, or use the older convention of prefixing such globals with an underscore (which you might want to do to indicate these globals are "module non-public").

This package is not designed to be used via from <module> import * thus I think we don't need the __all__ mechanism. Also we will need to keep the this list up-to-date, and since it doesn't add any value I suggest we don't use it.

We are utilizing __all__ to help VS Code’s Intellisense resolve the imported modules when developing. The main benefit provided is an improved developer experience as the IDE can automatically provide a list of available events for autocompletion.

MousaZeidBaker commented 2 years ago

We are utilizing __all__ to help VS Code’s Intellisense resolve the imported modules when developing. The main benefit provided is an improved developer experience as the IDE can automatically provide a list of available events for autocompletion.

The modules are perfectly resolved in my VS Code and the docstring is also provided, see screenshot below. You need to set the python interpreter correctly, see bottom left corner.

image

tekdj7 commented 2 years ago

We are utilizing __all__ to help VS Code’s Intellisense resolve the imported modules when developing. The main benefit provided is an improved developer experience as the IDE can automatically provide a list of available events for autocompletion.

The modules are perfectly resolved in my VS Code and the docstring is also provided, see screenshot below. You need to set the python interpreter correctly, see bottom left corner.

image

First of all, appreciate that your diving deep with me on this. Thank you. My python interpreter is correctly setup. When I hover over events.sqs.SQSMessage, it works fine. However, events.SQSEvent and context_.Context are marked as a problem in Visual Studio Code, and when you hover over it, you see the error related to X is not exported from module.. See screenshot below:

image

Also. Intellisense doesn't doesn't work, when I do events., in screenshot below you will see what comes up, but what is missing is the event names, such as SQSEvent.

image

For example, by including the exports via `all, errors go away, and Intellisense works properly, see screenshots below:

image

image

I am using Pylance, which is the default language server for Python in Visual Studio Code. I made all of the other suggestions you provided in the other threads, it all worked good.

MousaZeidBaker commented 2 years ago

No worries, I'm happy to assist. And just to be clear, I'm only trying to do the right thing and if something is needed then of course we can add it.

I haven't had the issues you are mentioning. Intellisense as well as autocomplete works fine on my VS Code and I haven't changed the python.languageServer setting previously. I played around with this setting and when setting it to Pylance the autocomplete could sometimes (not always) disappear for some unknown reason but I never got any errors as you are showing. See demonstration below.

I dived deeper into Pylance and find out that they leverages on Pyright. Diving deeper into Pyright I found out that there are a set of rules that determines which symbols are visible outside a package. One is to use the __all__ symbol, another is to use symbols with a redundant module alias from .X import A as A. I tried the later and it seemed to always work properly with Pylance so this could be our solution. Can you try this and see if it works with your machine? In the events __init__ change from .sqs import SQSEvent to from .sqs import SQSEvent as SQSEvent.

With that said, I think we have a couple of options to try out. Also I think this should be a feature by itself with a dedicated PR and to not be included in this PR. This PR should only address adding a new event namely CloudFormationEvent. This way we keep PRs isolated.

Peek 2021-10-29 00-00

tekdj7 commented 2 years ago

Sounds good. In the events __init__changed from .sqs import SQSEvent to from .sqs import SQSEvent as SQSEvent, and it worked. I updated this PR to be only the CloudFormation event entries. FYI, also included in 2 files, entries related to AppSyncResolverEvent, to address Merge Conflict.

I will create a separate PR for the exports to support a better user experience with Visual Studio Code, per your suggested method once you get this solution merged. Thanks for the continued support.