Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static typing: Logger __getattr__ definition causes mypy to not error when missing an attribute #1660

Closed
nayaverdier opened this issue Oct 27, 2022 · 4 comments · Fixed by #1670
Assignees
Labels
typing Static typing definition related issues (mypy, pyright, etc.)

Comments

@nayaverdier
Copy link
Contributor

Static type checker used

mypy (project's standard)

AWS Lambda function runtime

3.9

AWS Lambda Powertools for Python version

latest

Static type checker info

The issue here is a lack of output. Running mypy with no arguments on the file below does not produce any issues.

Code snippet

from aws_lambda_powertools import Logger

Logger().foobar()

Possible Solution

Based on python/mypy#13319, the preferred solution seems to be the following:

from typing import TYPE_CHECKING

class Logger(logging.Logger):
    ...
    if not TYPE_CHECKING:
        def __getattr__(self, name):
            ...

This appears to resolve the issue on my machine.

@nayaverdier nayaverdier added triage Pending triage from maintainers typing Static typing definition related issues (mypy, pyright, etc.) labels Oct 27, 2022
@heitorlessa
Copy link
Contributor

hey @nayaverdier - thanks for flagging it! We'll need to look into it as it's slightly more complex than that.

There are two reasons we added it 2 years ago:

  • (1) PyCharm and VSCode didn't auto-complete methods like info(), warning() that were only available in self._logger
  • (2) Ease migration for those coming from std logger - add filters to the logger, etc.

If we do that suggestion not TYPE_CHECKING, this might break 1 and can break 2. Perhaps there's another way.

It's hard to know who's doing 2. Now that we're in V2 and Mypy strict typing is in the roadmap, we could look into having a deprecated warning or something if we can't provide type safety by the end of strict typing changes.

@nayaverdier
Copy link
Contributor Author

nayaverdier commented Oct 28, 2022

@heitorlessa Thanks for the info. I imagine since Logger subclasses logging.Logger again since #110, (1) might not actually be an issue? I also added print("Proxying:", name) to __getattr__ so we can see what is being proxied to _logger:

from aws_lambda_powertools import Logger

logger = Logger(service="test")
logger.info("test")

Yields the following output:

Proxying: disabled
Proxying: _cache
Proxying: level
Proxying: level
Proxying: _cache
Proxying: name
Proxying: disabled
Proxying: filters
Proxying: handlers
{"level":"INFO","location":"<module>:1","message":"hi","timestamp":"2022-10-28 10:29:44,944-0700","service":"service_undefined"}
Proxying: propagate
Proxying: parent

Using Pyright in vim (which should be very similar to Pylance in VSCode) and with the if not TYPE_CHECKING change, I am seeing all of these attributes autocompleting (and all the info etc methods just from the subclassing), which I think covers (2) unless I am misunderstanding that. Can't speak to Pycharm, though.

@heitorlessa
Copy link
Contributor

Ah great! If you're seeing in vim, we can then test removing it as a scream test - if anyone complains (Hyrum's Law) we can revert quickly.

With the registered_formatter and registered_handler properties, I think it already addresses 99% the needs for accessing something in _logger.

We're making a release on Monday (PyPi prod was in maintenance today).

Would you like to make a PR to remove it? I can merge and update release notes before releasing it

Thank you!

@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Oct 31, 2022
@heitorlessa heitorlessa self-assigned this Oct 31, 2022
@heitorlessa heitorlessa linked a pull request Oct 31, 2022 that will close this issue
5 tasks
@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Static typing definition related issues (mypy, pyright, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants