Skip to content

Conversation

@oars
Copy link

@oars oars commented Apr 22, 2025

No description provided.

@jonathanj
Copy link
Owner

Thanks for the PR, I just have 2 comments:

  1. Can you name the argument and all its uses use_microseconds (note the plural) instead? This is more idiomatic English, and what I would expect the option to be named as a user
  2. Can you add tests to test_cli (integration tests exercising the command line option) and test_render (unit tests exercising the implementation) for the microseconds cases?

@oars
Copy link
Author

oars commented May 5, 2025

Done.

@jonathanj
Copy link
Owner

If I run pytest I see the following test failures, that I don't see on master. Can you take a look at why there are test failures?

======================================================================================== short test summary info =========================================================================================
FAILED src/eliottree/test/test_format.py::TimestampTests::test_local - Failed: NOTE: Incompatible Exception Representation, displaying natively:
FAILED src/eliottree/test/test_render.py::DefaultValueFormatterTests::test_timestamp_field_local - Failed: NOTE: Incompatible Exception Representation, displaying natively:
FAILED src/eliottree/test/test_render.py::DefaultValueFormatterTests::test_timestamp_microseconds - Failed: NOTE: Incompatible Exception Representation, displaying natively:
=============================================================================== 3 failed, 92 passed, 16 warnings in 2.19s ================================================================================

@oars
Copy link
Author

oars commented May 7, 2025

Hmmm that's annoying. I was seeing failures locally before my changes which is why I switched some timezone stuff to constants, but clearly that wasn't enough.

I added more explicit mocking of datetime to prevent that now. Could you check if it's good on your side?

@jonathanj
Copy link
Owner

Is it necessary to mock timezones? The existing tests didn't mock timezones and I don't think microseconds require timezones to function, what were the timezone/mock changes to support?

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