Skip to content

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

Open
Sooshiance wants to merge 4 commits into
marselester:masterfrom
Sooshiance:master
Open

feature/type-checker-compatible-with-mypy#38
Sooshiance wants to merge 4 commits into
marselester: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

Sooshiance commented Feb 28, 2025 via email

Copy link
Copy Markdown
Author

@Sooshiance

Sooshiance commented Jun 4, 2025

Copy link
Copy Markdown
Author

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