-
-
Notifications
You must be signed in to change notification settings - Fork 622
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
Default tag ordering to the primary key #892
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ltering on a model instance
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #892 +/- ##
==========================================
- Coverage 92.31% 92.16% -0.16%
==========================================
Files 9 9
Lines 729 740 +11
Branches 131 135 +4
==========================================
+ Hits 673 682 +9
- Misses 36 37 +1
- Partials 20 21 +1 ☔ View full report in Codecov by Sentry. |
@@ -41,7 +41,7 @@ exclude = tests* | |||
[flake8] | |||
# E501: line too long | |||
ignore = E501 | |||
exclude = .venv,.git,.tox | |||
exclude = .venv,.git,.tox,.direnv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
been meaning to add this for a while....
taggit/managers.py
Outdated
seen_tags = set() | ||
for t in tags: | ||
if isinstance(t, self.through.tag_model()): | ||
if t in seen_tags: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicky but could be simpler here to do:
if not t in seen_tags:
seen_tags.add(t)
result.append(t)
taggit/managers.py
Outdated
tag_objs.update(existing) | ||
# confirm if we've seen it or not (this is where case insensitivity comes | ||
# into play) | ||
if existing_tag in seen_tags: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise we can eliminate the continue here
@rtpg thanks for putting this together! I left two minor comments but I think it looks great and is a cleaner refactor! Would love to see this merged in. I appreciate the offer! Would love to be included on the |
@@ -19,3 +19,4 @@ Andrew Pryde <andrew@rocketpod.co.uk> | |||
John Whitlock <jwhitlock@mozilla.com> | |||
Jon Dufresne <jon.dufresne@gmail.com> | |||
Pablo Olmedo Dorado <pablolmedorado@gmail.com> | |||
Steve Reciao <stevenrecio@gmail.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*Recio 😎
This is based off of work done by @steverecio on #871
This doesn't cause a change in query count.
@steverecio would you mind looking over this? And do you want to be placed in the
AUTHORS
file? I appreciate you digging through this, your PR was very helpful and helped me to understand what needed to be done to get this working.