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

bug: mypy from pre-commit hook found multiple type-related errors #294

Closed
introkun opened this issue Apr 5, 2024 · 7 comments · Fixed by #318
Closed

bug: mypy from pre-commit hook found multiple type-related errors #294

introkun opened this issue Apr 5, 2024 · 7 comments · Fixed by #318
Labels

Comments

@introkun
Copy link
Contributor

introkun commented Apr 5, 2024

What version of MyFinances are you currently using?

v0.3.0

What device type are you currently facing the issue on?

Desktop

Describe the bug

  1. Fix all errors
  2. Enable back mypy for settings and backend folders
- hook id: mypy
- exit code: 1

settings/local_settings.py:43: error: Dict entry 1 has incompatible type "str": "Path"; expected "str": "dict[str, str] | str | int"  [dict-item]
settings/helpers.py:100: error: Name "response" already defined on line 97  [no-redef]
settings/helpers.py:119: error: Missing positional argument "response" in call to "SentEmailErrorResponse"  [call-arg]
settings/settings.py:32: error: Incompatible import of "ALLOWED_HOSTS" (imported name has type "list[str | None]", local name has type "list[str]")  [assignment]
settings/settings.py:92: error: Need type annotation for "EMAIL_WHITELIST" (hint: "EMAIL_WHITELIST: List[<type>] = ...")  [var-annotated]
settings/settings.py:362: error: Name "CustomPublicMediaStorage" already defined on line 286  [no-redef]
settings/settings.py:373: error: Name "CustomPrivateMediaStorage" already defined on line 324  [no-redef]
backend/models.py:373: error: Incompatible types in assignment (expression has type "float | Decimal", variable has type "float")  [assignment]
backend/models.py:395: error: Incompatible types in assignment (expression has type "int", variable has type "Decimal")  [assignment]
backend/models.py:397: error: Unsupported operand types for - ("Decimal" and "float")  [operator]
backend/models.py:397: error: Argument 1 to "get_tax" of "Invoice" has incompatible type "Decimal"; expected "float"  [arg-type]
backend/models.py:624: error: Missing positional argument "x" in call to "count" of "str"  [call-arg]
backend/models.py:642: error: Item "None" of "QuotaLimit | None" has no attribute "filter"  [union-attr]
backend/models.py:647: error: Item "None" of "QuotaLimit | None" has no attribute "filter"  [union-attr]
backend/models.py:649: error: Item "None" of "QuotaLimit | None" has no attribute "filter"  [union-attr]
backend/models.py:655: error: Implicit return in function which does not return  [misc]
backend/models.py:661: error: Missing positional argument "x" in call to "count" of "str"  [call-arg]
backend/models.py:662: error: Item "str" of "Any | Literal['Not Available']" has no attribute "filter"  [union-attr]
backend/models.py:663: error: Item "str" of "Any | Literal['Not Available']" has no attribute "filter"  [union-attr]
backend/models.py:675: error: Missing positional argument "x" in call to "count" of "str"  [call-arg]
backend/models.py:676: error: Item "str" of "Any | Literal['Not Available']" has no attribute "order_by"  [union-attr]
backend/models.py:680: error: Item "str" of "Any | Literal['Not Available']" has no attribute "first"  [union-attr]
Found 22 errors in 4 files (checked 2 source files)
@Domejko
Copy link
Contributor

Domejko commented Apr 11, 2024

Hey @TreyWW

Will you want to implement mypy or no ? If you do then I can take care of this.
But it's kind of strange because when it here run as a hook it shows only 22 errors but when I have checked on my machine with mypy 1.7.1 it did found almost 500 errors by running mypy backend/ and mypy settings/ commands. I did reduced error count to 374 atm by installing django stub packages and overriding some missing import errors.
In this case then should I fix only those 22 errors from pre-commit mypy and ignore those from running mypy locally or the opposite ?

@TreyWW
Copy link
Owner

TreyWW commented Apr 11, 2024

Hey @Domejko,

Yeah I definitely do want to go down the route of adding MyPy, I think in general it would make the project a lot cleaner and easier to understand.

I don't plan on adding MyPy to pre-commit hooks back though, since using PreCommit hooks in general my productivity has gone downhill so bad, I've literally gave up committing some features just because of the pain it's causing.

But getting them MyPy errors down to 0 should definitely be a goal either way.

If you wish, you could just make a PR for the changes you've made to get that number down, and instead of trying to do all errors in one go, we can slowly get them down each time and just do them day by day until we reach 0.

Does that sound like a good plan? Let me know if you've got any other ideas, thanks for your contribution :)

@Domejko
Copy link
Contributor

Domejko commented Apr 11, 2024

Yes I agree hook didn't even cover all the files correctly apparently so I guess it's no use.

I will add then necessary packages and soon will send a PR with a fixes that I have made today and will start working on the rest of errors step by step daily. When it all will be fixed I think then we can add mypy to the contributing procedure to avoid piling up errors in the future.

One more thing, should I pass this topic in the PRs or we close it and I'll send them as non-related to any issue ?

@TreyWW
Copy link
Owner

TreyWW commented Apr 11, 2024

Amazing, that's a great idea!

One more thing, should I pass this topic in the PRs or we close it and I'll send them as non-related to any issue ?

Yeah you can include this as closes #294 since this issue isn't too relevant now we no longer use mypy as a pre-commit hook.

Thanks!

@Domejko
Copy link
Contributor

Domejko commented Apr 11, 2024

@TreyWW when I try to run tests I'm getting this error:

  File "/home/domejko/MyFinances/infrastructure/aws/handler.py", line 62, in get_iam_client
    raise ValueError("If using schedules, the variables MUST be set.")
ValueError: If using schedules, the variables MUST be set.

Do you know maybe some quick solution for that ?

@TreyWW
Copy link
Owner

TreyWW commented Apr 11, 2024

You'll want to disable schedules. Set both isInvoiceSchedulingEnabled and areInvoiceRemindersEnabled feature flags off. You'll need to do this in the "Feature Flags" table of the DB

@introkun
Copy link
Contributor Author

isInvoiceSchedulingEnabled is off in the database and I have same issue @TreyWW :

  File "/Users/sergey/projects/MyFinances/backend/urls.py", line 31, in <module>
    path("api/", include("backend.api.urls")),
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sergey/opt/miniconda3/envs/py3122-myfinance/lib/python3.12/site-packages/django/urls/conf.py", line 39, in include
    urlconf_module = import_module(urlconf_module)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sergey/opt/miniconda3/envs/py3122-myfinance/lib/python3.12/importlib/__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 995, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "/Users/sergey/projects/MyFinances/backend/api/urls.py", line 11, in <module>
    path("invoices/", include("backend.api.invoices.urls")),
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sergey/opt/miniconda3/envs/py3122-myfinance/lib/python3.12/site-packages/django/urls/conf.py", line 39, in include
    urlconf_module = import_module(urlconf_module)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sergey/opt/miniconda3/envs/py3122-myfinance/lib/python3.12/importlib/__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 995, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "/Users/sergey/projects/MyFinances/backend/api/invoices/urls.py", line 3, in <module>
    from . import fetch, delete, edit, schedule, manage
  File "/Users/sergey/projects/MyFinances/backend/api/invoices/schedule.py", line 20, in <module>
    from infrastructure.aws.schedules.create_schedule import (
  File "/Users/sergey/projects/MyFinances/infrastructure/aws/schedules/create_schedule.py", line 10, in <module>
    from infrastructure.aws.iam.sfn import get_sfn_execute_role_arn
  File "/Users/sergey/projects/MyFinances/infrastructure/aws/iam/sfn.py", line 9, in <module>
    iam_client = get_iam_client()
                 ^^^^^^^^^^^^^^^^
  File "/Users/sergey/projects/MyFinances/infrastructure/aws/handler.py", line 62, in get_iam_client
    raise ValueError("If using schedules, the variables MUST be set.")
ValueError: If using schedules, the variables MUST be set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants