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.

Sign up for GitHub

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

Jump to bottom

Refs #28643 -- Added MD5 database function. #11004

Merged
merged 1 commit into from Feb 21, 2019
Merged

Refs #28643 -- Added MD5 database function. #11004

merged 1 commit into from Feb 21, 2019

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Feb 18, 2019

Ticket 28643.

charettes
charettes reviewed Feb 18, 2019
View reviewed changes
tests/db_functions/text/test_md5.py Outdated Show resolved Hide resolved
tests/db_functions/text/test_md5.py Outdated Show resolved Hide resolved
tests/db_functions/text/test_md5.py Outdated Show resolved Hide resolved
ngnpope
ngnpope suggested changes Feb 18, 2019
View reviewed changes
Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

Yay. Thanks for this. Nice Oracle-fu too.

Please can you add a test for the NULL case? This is particularly important for the SQLite bit. (I realise that we still need to add these NULL tests for some of the other functions.)

django/db/models/functions/text.py Outdated Show resolved Hide resolved
django/db/models/functions/text.py Outdated Show resolved Hide resolved
docs/ref/models/database-functions.txt Outdated Show resolved Hide resolved
@ngnpope ngnpope mentioned this pull request Feb 19, 2019
Closed
@django django deleted a comment Feb 19, 2019
@felixxm felixxm force-pushed the refs-28643-md5 branch 2 times, most recently from f8e9ac8 to 209e542 Compare February 19, 2019 07:25
@felixxm
Copy link
Member Author

felixxm commented Feb 19, 2019

@charettes @pope1ni Thanks for reviews 👍 . I updated PR.

ngnpope
ngnpope approved these changes Feb 19, 2019
View reviewed changes
Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

Looks good to me.

django/db/models/functions/__init__.py Outdated Show resolved Hide resolved
timgraham
timgraham approved these changes Feb 20, 2019
View reviewed changes
django/db/models/functions/__init__.py
@@ -10,8 +10,9 @@
Mod, Pi, Power, Radians, Round, Sin, Sqrt, Tan,
)
from .text import (
Chr, Concat, ConcatPair, Left, Length, Lower, LPad, LTrim, Ord, Repeat,
Replace, Reverse, Right, RPad, RTrim, StrIndex, Substr, Trim, Upper,
MD5, Chr, Concat, ConcatPair, Left, Length, Lower, LPad, LTrim, Ord,
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why isort sorts M before C. Is there a special rule for names in all caps?

Copy link
Member Author

Choose a reason for hiding this comment

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

isort treats MD5 as a constant and sorts imports for CONSTANTS, Classes, and modules/functions (see order_by_type setting).

docs/ref/models/database-functions.txt Show resolved Hide resolved
docs/ref/models/database-functions.txt Outdated Show resolved Hide resolved
@felixxm
Refs #28643 -- Added MD5 database function.
0382a3b 
Thanks Tim Graham, Nick Pope and Simon Charette for reviews.
@felixxm felixxm merged commit 9ff18c0 into django:master Feb 21, 2019
@felixxm felixxm deleted the refs-28643-md5 branch February 21, 2019 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charettes charettes charettes left review comments

@timgraham timgraham timgraham approved these changes

@ngnpope ngnpope ngnpope approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@felixxm @charettes @timgraham @ngnpope