Skip to content

Conversation

@dliappis
Copy link
Contributor

@dliappis dliappis commented Jun 25, 2025

This commit fixes a typo observed in
#172 (comment)

Also allows to simply the code without a need for a helper function.

@dliappis dliappis requested a review from xrmx June 25, 2025 14:00
@dliappis dliappis self-assigned this Jun 25, 2025
@dliappis dliappis marked this pull request as draft June 25, 2025 14:01
@dliappis dliappis force-pushed the fix-mypy-again branch 2 times, most recently from 7a192ed to b4eb92b Compare June 25, 2025 14:12
This commit fixes a mistake observed in
elastic#172 (comment)

and allows to simply the code further (no need for a helper function)
- id: check-bash-syntax
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.910
rev: v1.16.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that we are currently installing the latest version of mypy when executing the various Python 3.x tests using nox (e.g. see https://github.com/elastic/ecs-logging-python/blob/main/noxfile.py#L54) hence there was a discrepancy here when running precommit tests, that was surfacing non-existent errors.

@dliappis dliappis marked this pull request as ready for review June 25, 2025 14:54
@dliappis
Copy link
Contributor Author

@xrmx please take a look at this PR when you can; I think it fixes the typo/issue you observed in #172 and also various mypy issues due to different versions used in different tests.

xrmx
xrmx previously approved these changes Jun 26, 2025
Copy link
Member

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround!

@xrmx xrmx enabled auto-merge (squash) June 26, 2025 07:19
@xrmx xrmx merged commit 5941904 into elastic:main Jun 26, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants