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

Mypy errors fix #373

Merged
merged 6 commits into from
May 21, 2024
Merged

Mypy errors fix #373

merged 6 commits into from
May 21, 2024

Conversation

Domejko
Copy link
Contributor

@Domejko Domejko commented May 20, 2024

Description

Regarding #319

  • reduced error count from 28 to 7
  • In backend/api/invoices/create/services/add.py existing_service default value have been change to 0 because empty string as id was causing ValueError

Edit:

  • all errors have been resolved
    Screenshot from 2024-05-21 13-57-27
  • In pyproject.toml added options for mypy to ignore environment folders. Set incremental option to false due to this error. Disabled note. --explicit_package_bases flag have also now been setup in pyproject.toml so current command to run checks is mypy .

Issue #319 will be closed

Checklist

  • Ran the Black Formatter and
    djLint-er on any new code
    (checks
    will
    fail without)
  • Made any changes or additions to the documentation where required
  • Changes generate no new warnings/errors
  • New and existing unit tests pass locally with my
    changes

What type of PR is this?

  • 🐛 Bug Fix
  • ♻️ Code Refactor

Added/updated tests?

  • 🙅 no, because they aren't needed

Related PRs, Issues etc

@Domejko
Copy link
Contributor Author

Domejko commented May 20, 2024

Could you please run test for those changes on your machine, since when I was trying to test this code on mine at the end it just freeze and after a while restart system. Now I'm not sure is it my machine or code because in github actions everything is passing fine with out any issues.

backend/models.py Outdated Show resolved Hide resolved
@@ -9,7 +9,7 @@
@require_http_methods(["POST"])
def add_service(request: HtmxHttpRequest):
context: dict = {}
existing_service = request.POST.get("existing_service", "")
existing_service = request.POST.get("existing_service", 0)
Copy link
Owner

Choose a reason for hiding this comment

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

Just a bit confused why this should default to 0. POST.get will always return a string. I'm guessing in this case you want 0 to just represent None, but wouldn't it make more sense and be better to just use None? I'm confused how setting it to an integer fixes the errors lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yes, I just thought of the way to trigger exception and not a ValueError, None probably is a cleaner solution. I will change that since I found also few more issues of this type in that file.
You want me to send them as seperate PR or together with mypy contribution docs update, CI, pre-commit hook ?

Copy link
Owner

Choose a reason for hiding this comment

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

Separate PR please, thanks. The smaller the PR the quicker I can merge. Otherwise it'll sit there for a month lol

Copy link
Contributor Author

@Domejko Domejko May 22, 2024

Choose a reason for hiding this comment

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

Hmm now as I have dug deeper into this existing_service = request.POST.get("existing_service", 0) the expected values by existing_service_obj = InvoiceProduct.objects.get(user=request.user, id=existing_service) are of a type str | int so even tho with None it works fine, in terms of type annotations None is a invalid value and 0 or "0" is a valid one. In this case I think that leaving it as 0 will be cleaner than placing # type: ignore since valid product.id never should be a 0.

@TreyWW TreyWW merged commit 83a04dc into TreyWW:main May 21, 2024
11 checks passed
@Domejko Domejko deleted the mypy-errors-fix branch May 22, 2024 08:31
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.

MyPy Error Fix | Open to all
2 participants