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

Misc. Django/Python improvements #1050

Merged
merged 3 commits into from
Apr 22, 2022
Merged

Conversation

mvandenburgh
Copy link
Member

I noticed a bunch of code that did things in a slightly incorrect and/or outdated way, this PR fixes them -

  1. Updates any use of .filter(~Q(...)) to .exclude(...). exclude behaves the same and is the "correct" way to do this sort of query in Django
  2. Update DRF viewsets to use rest_framework.generics.get_object_or_404 instead of django.shortcuts.get_object_or_404.
  3. Replace instances of typing.Union and typing.Optional with new union type syntax

Technically, 3) isn't necessarily incorrect (from what I understand, typing.Optional and typing.Union are not deprecated, at least not yet), but I find the x | y union syntax to be much more readable. I don't feel strongly on this though 🤷‍♂️

@jjnesbitt jjnesbitt added enhancement New feature or request maintenance Action to maintain the system (neither a bugfix nor an enhancement) and removed enhancement New feature or request labels Apr 20, 2022
Copy link
Member

@jjnesbitt jjnesbitt left a comment

Choose a reason for hiding this comment

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

LGTM

@mvandenburgh mvandenburgh merged commit fe7b79c into master Apr 22, 2022
@mvandenburgh mvandenburgh deleted the misc-django-improvements branch April 22, 2022 16:57
@dandibot
Copy link
Member

🚀 PR was released in v0.2.18 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Action to maintain the system (neither a bugfix nor an enhancement) released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants