Switch base methods to return Self instead of BoundLoggerBase by aThorp96 · Pull Request #659 · hynek/structlog

aThorp96 · 2024-10-03T20:51:10Z

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:

After this change:

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 · 2024-10-03T21:38:39Z

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 · 2024-10-04T04:10:05Z

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 · 2024-10-04T14:01:13Z

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 · 2024-10-04T14:53:52Z

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 · 2024-10-04T15:04:11Z

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

for more information, see https://pre-commit.ci

aThorp96 · 2024-10-05T00:42:30Z

tests/typing/api.py



+def typecheck_bound_logger_return() -> None:
+    blogger: structlog.BoundLogger = structlog.get_logger(__name__)


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.

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.

for more information, see https://pre-commit.ci

src/structlog/_base.py

hynek

Thanks!

Switch base methods to return Self instead of BoundLoggerBase

c04f3ec

aThorp96 and others added 2 commits October 4, 2024 20:40

add tests for BoundLoggerBase return type

0b27620

[pre-commit.ci] auto fixes from pre-commit.com hooks

949e5c4

for more information, see https://pre-commit.ci

aThorp96 commented Oct 5, 2024

View reviewed changes

aThorp96 and others added 10 commits October 4, 2024 20:49

re-export version-safe Self in typings

fa43344

workaround self-assignment (pun intended)

a4c93af

switch _self import name to be consistent with class _Self

9206ef4

fix (generic) BoundLogger api test to only call generic methods

abb9e6e

remove re-export of Self due to typealias, state issues

892b55f

[pre-commit.ci] auto fixes from pre-commit.com hooks

94d5778

for more information, see https://pre-commit.ci

fix redundant import rename

e5fde14

fix redundant import rename

5e21e43

remove no-longer-necessary type ignores!

bf8e6c3

remove moar no-longer-necessary type-ignores!

d8041b2

hynek reviewed Oct 5, 2024

View reviewed changes

src/structlog/_base.py Show resolved Hide resolved

hynek approved these changes Oct 7, 2024

View reviewed changes

hynek merged commit c53f148 into hynek:main Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Switch base methods to return Self instead of BoundLoggerBase#659

Switch base methods to return Self instead of BoundLoggerBase#659
hynek merged 13 commits intohynek:mainfrom
aThorp96:switch-error-type-narrowing

aThorp96 commented Oct 3, 2024 •

edited

Loading

Uh oh!

aThorp96 commented Oct 3, 2024

Uh oh!

hynek commented Oct 4, 2024

Uh oh!

aThorp96 commented Oct 4, 2024

Uh oh!

hynek commented Oct 4, 2024

Uh oh!

aThorp96 commented Oct 4, 2024

Uh oh!

aThorp96 Oct 5, 2024

Uh oh!

aThorp96 Oct 5, 2024 •

edited

Loading

Uh oh!

Uh oh!

hynek left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



		def typecheck_bound_logger_return() -> None:
		blogger: structlog.BoundLogger = structlog.get_logger(__name__)

Uh oh!

Conversation

aThorp96 commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Pull Request Check List

Uh oh!

aThorp96 commented Oct 3, 2024

Uh oh!

hynek commented Oct 4, 2024

Uh oh!

aThorp96 commented Oct 4, 2024

Uh oh!

hynek commented Oct 4, 2024

Uh oh!

aThorp96 commented Oct 4, 2024

Uh oh!

aThorp96 Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

aThorp96 Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hynek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aThorp96 commented Oct 3, 2024 •

edited

Loading

aThorp96 Oct 5, 2024 •

edited

Loading