fix(logger): add explicit None return type annotations

[FollowTheProcess]

Issue number: #3112

Summary

Improves the type annotations in the logging module, specifically around those functions/methods that return None (with some bonus low hanging fruit elsewhere); solving the issue raised in #3112.

However, this has revealed a few new typing issues that weren't present before; some of which were simple fixes so I have just made them. Others I have questions on how we want to handle which I will post as comments below.

Changes

Please provide a summary of what's being changed

Added explicit -> None return type annotations where appropriate in the logging module

User experience

Please share what the user experience looks like before and after this change

Should mean that users using mypy in strict mode no longer have to suppress type errors on certain logging functions/methods

Checklist

If your change doesn't seem to apply, please leave them unchecked.

• Meet tenets criteria
• I have performed a self-review of this change
• Changes have been tested
• Changes are documented
• PR title follows conventional commit semantics

Is this a breaking change?

RFC issue number:

Checklist:

• Migration process documented
• Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

[boring-cyborg]

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

[FollowTheProcess]

This revealed a few issues with mypy that didn't occur when I first checked the repo out:

It looks as if the registered_handler property accesses self._logger.parent.handlers even though technically self._logger.parent returns Logger | None

Now I'm guessing but the self.child attribute seems to encode the info that there is indeed a parent so this looks more like mypy can't understand that from context? Just wondering on the best way you'd like to address this? I did try changing if self.child condition to if self._logger.parent is not None which reads the same but it broke a lot of tests so there must be something else going on there.

The second issue is that mypy now appears not to like the _get_caller_filename at all as inspect.currentframe() again returns FrameType | None so each call to f_back could be None which we're not handling in that function:

Again this seems like a case where we know better than mypy to me and it's not getting the full context as we know by definition (and the comment) that there will always be at least 3 frames.

Are we happy with # type: ignore in these cases? Or do we want to explicitly handle the possibility of None (however unlikely)

[heitorlessa]

This revealed a few issues with mypy that didn't occur when I first checked the repo out:

It looks as if the registered_handler property accesses self._logger.parent.handlers even though technically self._logger.parent returns Logger | None

Now I'm guessing but the self.child attribute seems to encode the info that there is indeed a parent so this looks more like mypy can't understand that from context? Just wondering on the best way you'd like to address this? I did try changing if self.child condition to if self._logger.parent is not None which reads the same but it broke a lot of tests so there must be something else going on there.

The second issue is that mypy now appears not to like the _get_caller_filename at all as inspect.currentframe() again returns FrameType | None so each call to f_back could be None which we're not handling in that function:

Again this seems like a case where we know better than mypy to me and it's not getting the full context as we know by definition (and the comment) that there will always be at least 3 frames.

Are we happy with # type: ignore in these cases? Or do we want to explicitly handle the possibility of None (however unlikely)

The former - I'd be happy with adding type: ignore[Specific_Error_to_Ignore]. I'm genuinely nervous with a blank type: ignore as it could miss on other future regressions vs ignoring the None instance only.

The main reason we didn't go with strict mode in Mypy is that it ended up causing more trouble than saving the effort. One item in the back of my mind is to take yet another look at Pyright -- it was better at these things, but I wasn't sure if it was going to be maintained long enough after all those years (it did!!).

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03% ⚠️

Comparison is base (2f38c94) 96.13% compared to head (8c40bcc) 96.11%.
Report is 2 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3113      +/-   ##
===========================================
- Coverage    96.13%   96.11%   -0.03%     
===========================================
  Files          186      186              
  Lines         8131     8131              
  Branches      1525      618     -907     
===========================================
- Hits          7817     7815       -2     
- Misses         252      253       +1     
- Partials        62       63       +1     

Files Changed	Coverage Δ
aws_lambda_powertools/logging/filters.py	100.00% <100.00%> (ø)
aws_lambda_powertools/logging/formatter.py	100.00% <100.00%> (ø)
aws_lambda_powertools/logging/logger.py	99.45% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[heitorlessa]

LGTM pending CI results -- lemme know otherwise if there's any other change.

Appreciate those extra comments and explicit ignore types 🫶

Making a release tomorrow

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[boring-cyborg]

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

[heitorlessa]

Released in 2.25.1 in PyPi. Building and optimizing Lambda Layers now -- should be all done in ~20m or so. Next Layer version being 45.