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

Switch base methods to return Self instead of BoundLoggerBase #659

Merged
merged 13 commits into from
Oct 7, 2024

Conversation

aThorp96
Copy link
Contributor

@aThorp96 aThorp96 commented Oct 3, 2024

Summary

This Pull Request switches several BoundLoggerBase methods to annotate a return type of typing_extensions.Self instead of BoundLoggerBase. This change is consistent with the code which returns specifically self.__class__() and with the tests that ensure the type is preserved when .bind() is called.

This should not have any runtime changes, but instead fixes a typing bug which caused the following code to incorrectly fail type-checking:

from structlog import get_logger, BoundLogger

log: BoundLogger = get_logger()
log.info("This is OK")
log = log.bind() #  error: Type "BoundLoggerBase" is not assignable to declared type "BoundLogger"
log.info("This should also be okay")

Before this change:
Screenshot 2024-10-03 at 4 48 17 PM

After this change:
Screenshot 2024-10-03 at 4 48 54 PM

Pull Request Check List

It does not appear to me that any of these apply, but I am happy to make any necessary changes to the PR or do any upkeep tasks

  • Do not open pull requests from your main branch – use a separate branch!
    • There's a ton of footguns waiting if you don't heed this warning. You can still go back to your project, create a branch from your main branch, push it, and open the pull request from the new branch.
    • This is not a pre-requisite for your pull request to be accepted, but you have been warned.
  • Added tests for changed code.
    • The CI fails with less than 100% coverage.
  • New APIs are added to our typing tests in api.py.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      • The next version is the second number in the current release + 1. The first number represents the current year. So if the current version on PyPI is 23.1.0, the next version is gonna be 23.2.0. If the next version is the first in the new year, it'll be 24.1.0.
  • Documentation in .rst and .md files is written using semantic newlines.
  • Changes (and possible deprecations) are documented in the changelog.
  • Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

@aThorp96
Copy link
Contributor Author

aThorp96 commented Oct 3, 2024

All of the CI here is failing because it cannot import typing_extensions. Not sure what's going on with that; typing_extensions was already a dependency...

@hynek
Copy link
Owner

hynek commented Oct 4, 2024

typing extensions needs to be a conditional import as done in #642 – and speaking of it, can you check please if your problems aren't fixed by that already?

If not, feel free to import Self from structlog.typing.

@aThorp96
Copy link
Contributor Author

aThorp96 commented Oct 4, 2024

Thanks @hynek! I missed that typing_extensions.Self was only available on python <3.11. It makes sense to only import it for those python versions. I've switched to structlog.typing.Self since it already handles the version-specific cases. Not sure why the PR isn't updating yet with the changes I've pushed.

Unfortunately #642 does not fix the type error I am seeing, since BoundLogger directly sublcasses BoundLoggerBase. The screenshot of the Pyright error is on main yesterday, on 9e9711c specifically.

@hynek
Copy link
Owner

hynek commented Oct 4, 2024

In that case, thanks for catching it before shipping #642. :) Is there maybe a way to add a typing test to https://github.com/hynek/structlog/blob/main/tests/typing/api.py that would've caught it?

Either way, you haven't pushed you changes yet. 🤓

@aThorp96
Copy link
Contributor Author

aThorp96 commented Oct 4, 2024

I think github is being odd; aThorp96:switch-error-type-narrowing is up to date with the change. I rebased the branch on my end to squash the changes, and I think it messed w/ Github's remote ref to my branch.... I'll try to add a test to the API and open a new PR against the branch, since you can't change the PR's HEAD ref

@@ -323,6 +323,19 @@ async def typecheck_stdlib_async() -> None:
await logger.alog(logging.CRITICAL, "async log")


def typecheck_bound_logger_return() -> None:
blogger: structlog.BoundLogger = structlog.get_logger(__name__)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting find here, and perhaps the reason that this hasn't come up sooner:
This test would fail with structlog.BoundLogger before this PR, but it would pass with structlog.stdlib.BoundLogger.

Copy link
Contributor Author

@aThorp96 aThorp96 Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, after hitting this, I think this is actually the root of the issue I'm experiencing; I don't think I realized that in some places I was typing as a structlog.BoundLogger and other places I was typing as a structlog.stdlib.BoundLogger, and if I did, I did not expect there to be a difference. My assumption would have been that the former was a top-level export of the latter. At least then I think my type error woes are gone. I'll still get this PR over the finish line, since i think it's correct and valuable.

Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@hynek hynek merged commit c53f148 into hynek:main Oct 7, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants