Skip to content

fix: Update codebase to use newer django syntax #1656

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

Closed

Conversation

abhiabhi94
Copy link
Contributor

@abhiabhi94 abhiabhi94 commented Apr 20, 2021

  • all commands of django_extensions now inherit from a base command _BaseDjangoExtensionsCommand.
  • this helps in defining constant at a single place.(The _ is prefixed intentionally to indicate the class as an internal API.)
  • handle django41 deprecation warnings.
  • update some test code to use upgraded version of django code.

@@ -12,7 +12,7 @@
class Command(BaseCommand):
help = "Removes all python bytecode compiled files from the project."

requires_system_checks = False
requires_system_checks = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in newer version of django, this value should be list/tuple of Tags or __all__. You can check the documentation here.

For mypy linting to work well with backward comptability, I think one of the solution would be to do something of the form:

from typing import List, Tuple, Union

from django.core.checks import Tags

requires_system_checks: Union[bool, List[Tags], Tuple[Tags], str]

Instead of making this change everywhere in the codebase, I think it would be better to create some sort of BaseCommandand let all commands inherit from it.

Would it be acceptable? Any suggestions are welcome.

@abhiabhi94 abhiabhi94 force-pushed the fix/django41-warnings branch from 6c8bd1b to c40a59a Compare April 20, 2021 09:47
@abhiabhi94 abhiabhi94 force-pushed the fix/django41-warnings branch from c40a59a to bbb2503 Compare April 30, 2021 11:43
@abhiabhi94
Copy link
Contributor Author

abhiabhi94 commented Apr 30, 2021

I have had to make some changes to workaround mypy which might make the diff a bit more than it should have.

As of now, it seems that mypy doesn't necessarily work well with inherited class attributes, hence had to ignore it at certain places as a workaround.

The issue can be demonstrated by the example below:

from typing import List

class A:
    a: List[int]

class B:
    a = []  # <- mypy tends to complain about this

I have created an issue there as well python/mypy#10375, but am yet to receive a response. So, for the time being, I think we can use the type: ignore thing as a workaround.

@abhiabhi94 abhiabhi94 force-pushed the fix/django41-warnings branch 2 times, most recently from ab79938 to dcde75e Compare April 30, 2021 12:17
- all commands of django_extensions now inherit from a base command `_BaseDjangoExtensionsCommand`.
- this helps in defining constant at a single place.
- handle django41 deprecation warnings.
- update some test code to use upgraded version of django code.
@camilonova
Copy link
Member

Looks like a big change and the codebase has changed a lot since it was first proposed. I'm closing this. Thank you.

@camilonova camilonova closed this Jun 11, 2022
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.

3 participants