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 type errors fix #324

Merged
merged 20 commits into from
Apr 20, 2024
Merged

Mypy type errors fix #324

merged 20 commits into from
Apr 20, 2024

Conversation

Domejko
Copy link
Contributor

@Domejko Domejko commented Apr 12, 2024

Description

Fixed all type errors in settings/ folder reducing there number from 384 to 369 errors.

Edit:
Part of files in backend/ also have been fixed and errors count is reduced to 354.

Edit:
Mypy error count reduced from 354 to 313.

Created utils/ folder, refactored utils.py to different files and updated imports accordingly.
Added new subclass of HttpRequest reduce type errors as suggested in django-stubs documentation.

Edit:

ValueError: If using schedules, the variables MUST be set.

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

backend/types/emails.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Copy link
Owner

@TreyWW TreyWW left a comment

Choose a reason for hiding this comment

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

There's a few minor issues I found with this PR but overall well done! Thanks for the contribution, would appreciate some changes made though :)

settings/helpers.py Outdated Show resolved Hide resolved
settings/local_settings.py Outdated Show resolved Hide resolved
settings/local_settings.py Show resolved Hide resolved
settings/prod_settings.py Outdated Show resolved Hide resolved
@Domejko
Copy link
Contributor Author

Domejko commented Apr 14, 2024

New commit with requested changes have been pushed.

@Domejko Domejko requested a review from TreyWW April 14, 2024 09:32
@Domejko
Copy link
Contributor Author

Domejko commented Apr 15, 2024

If you approve recent changes I think we should merge those changes since this issue is open to all to participate so we need to avoid cases when someone else will try to handle errors that have been already resolved in this PR.

@TreyWW TreyWW mentioned this pull request Apr 19, 2024
1 task
@Domejko
Copy link
Contributor Author

Domejko commented Apr 19, 2024

So I think I'm going to hold off on correcting errors until #339 will be implemented.

@TreyWW
Copy link
Owner

TreyWW commented Apr 19, 2024

Thank you

@TreyWW
Copy link
Owner

TreyWW commented Apr 19, 2024

Thanks for the wait. PR #339 has been merged

@Domejko
Copy link
Contributor Author

Domejko commented Apr 19, 2024

Ok, tomorrow I'll resolve conflicts and as soon as this PR will be merged I will proceed with work on the rest of errors.

@TreyWW
Copy link
Owner

TreyWW commented Apr 19, 2024

Amazing, sounds good

@Domejko
Copy link
Contributor Author

Domejko commented Apr 20, 2024

Regarding changes. If there will come features in the future that will be using @login_required decorator then instead of regular HttpRequest should be used AuthenticatedHttpRequest from backend/utils/http_utils.py. This will prevent mypy from throwing error about AnonymousUser type.

Also what do you think about moving HtmxHttpRequest subclass from backend/types/htmx.py to backend/utils/http_utils.py. Or maybe other way around, moving AuthenticatedHttpRequest to backend/types/ as authenticated_http.py.

@TreyWW
Copy link
Owner

TreyWW commented Apr 20, 2024

All views are login required, we just use middleware instead of @login_required

@TreyWW
Copy link
Owner

TreyWW commented Apr 20, 2024

As for the second part of the comment, I'm not too sure. I feel like the utils file should be for logic, rather than type definitions. Wherever the HTMX request is we could add a user object to that? And then add a second one if we use htmx without login required? We could talk more on discord if you'd like, may be easier

@Domejko
Copy link
Contributor Author

Domejko commented Apr 20, 2024

Yes sure, my username is domejko_

Copy link
Owner

@TreyWW TreyWW left a comment

Choose a reason for hiding this comment

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

Amazing PR, thank you!

@TreyWW TreyWW merged commit 746c032 into TreyWW:main Apr 20, 2024
11 checks passed
@blocage blocage mentioned this pull request Apr 21, 2024
4 tasks
@TreyWW TreyWW mentioned this pull request Apr 23, 2024
4 tasks
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