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

Get TimeStamper working properly if datetime module is mocked before its instantiation #364

Merged
merged 5 commits into from
Nov 11, 2021

Conversation

moriyoshi
Copy link
Contributor

@moriyoshi moriyoshi commented Nov 8, 2021

Summary

Because the reference to either datetime.now() or datetime.utcnow() is retrieved during TimeStamper is being created, it gets into trouble when FreezeGun or any similar mechanism that patches datetime module is applied before the instantiation. This patchs proposes a modification to it so that the invocation to the "now" function will always be done with dereferences from the datetime module object, albeit the slight possible performance penalty.

Checklist

  • Added tests for changed code.
  • 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. Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) are documented in the changelog.

@@ -333,7 +333,17 @@ def _make_stamper(
if fmt is None and not utc:
raise ValueError("UNIX timestamps are always UTC.")

now = getattr(datetime.datetime, "utcnow" if utc else "now")
now: Callable[[], datetime.datetime]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • This declaration is not necessary, but I prefer the "Φ" to be obvious.

@moriyoshi moriyoshi force-pushed the fix/get-stamper-work-well-with-mocking branch from dc04145 to 1c5c779 Compare November 8, 2021 23:24
hynek
hynek previously approved these changes Nov 9, 2021
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.

I guess it's a slight performance regression but function calls are getting cheaper over the past releases. :)

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.

Looks like mypy is not happy:

src/structlog/processors.py:372: error: Argument 1 to "strftime" of "datetime" has incompatible type "Optional[str]"; expected "str"

@moriyoshi
Copy link
Contributor Author

Looks like mypy is not happy:
src/structlog/processors.py:372: error: Argument 1 to "strftime" of "datetime" has incompatible type "Optional[str]"; expected "str"

This looks like a false error because fmt should have been asserted as non-None by the preceding if block and I didn't touch the part.

@hynek
Copy link
Owner

hynek commented Nov 11, 2021

Looks like mypy is not happy:
src/structlog/processors.py:372: error: Argument 1 to "strftime" of "datetime" has incompatible type "Optional[str]"; expected "str"

This looks like a false error because fmt should have been asserted as non-None by the preceding if block and I didn't touch the part.

You're right, looks like a mypy bug!

@hynek hynek enabled auto-merge (squash) November 11, 2021 04:46
@hynek hynek merged commit 6650c69 into hynek:main Nov 11, 2021
@moriyoshi moriyoshi deleted the fix/get-stamper-work-well-with-mocking branch November 14, 2021 23:19
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