Skip to content

feature/type-checker-compatible-with-mypy#38

Open
Sooshiance wants to merge 4 commits intomarselester:masterfrom
Sooshiance:master
Open

feature/type-checker-compatible-with-mypy#38
Sooshiance wants to merge 4 commits intomarselester:masterfrom
Sooshiance:master

Conversation

@Sooshiance
Copy link
Copy Markdown

I've added type checker(compatible with mypy) to your project, I hope that finds you well

@marselester
Copy link
Copy Markdown
Owner

Thank you! I'll have a look when I have a chance.

Comment thread .pre-commit-config.yaml Outdated
@@ -0,0 +1,35 @@
repos:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't think we need pre commit hooks.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Will do

Comment thread mypy.ini
@@ -0,0 +1,14 @@
[mypy]
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you please move this into pyproject.toml, e.g., https://mypy.readthedocs.io/en/stable/config_file.html#example-pyproject-toml?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK

Comment thread mypy.ini Outdated
@@ -0,0 +1,14 @@
[mypy]
python_version = 3.11
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I guess python_version is not necessary, for instance, I don't see it in the https://github.com/fastapi/fastapi/blob/master/pyproject.toml#L125.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, I will task it as an important notice.

Comment thread tests.py
logger.setLevel(logging.DEBUG)
logging.propagate = False
# This should be logger, not logging or use `type: ignore` instead
logger.propagate = False
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Nice catch 👍. Let's remove the This should be logger ... comment.

Comment thread json_log_formatter/__init__.py Outdated
@@ -1,33 +1,37 @@
from __future__ import annotations # backward compatibility
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I've deprecated Python < v3.9, could you please rebase? I wonder if we still need from __future__ import annotations.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, sorry for late responding...

@Sooshiance
Copy link
Copy Markdown
Author

Sooshiance commented Feb 28, 2025 via email

@Sooshiance
Copy link
Copy Markdown
Author

Sooshiance commented Jun 4, 2025

I've made your desired changes. If you found any issues, send me a comment.

@marselester
Copy link
Copy Markdown
Owner

Thanks! Could you please have a look at the failing tests in Python 3.9?

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