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

Refs #28643 -- Reorganized database functions. #9227

Merged
merged 1 commit into from Oct 13, 2017

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Oct 11, 2017

No description provided.

Copy link
Member

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

It might be worth planning the organization of the docs and code as work on #28643 continues. Is "base.py" moving toward "misc.py"? Should "ref/models/database-functions.html" headers be organized alphabetically or in some other fashion? (currently "Text Functions" is before "Date Functions"). Also, I noticed "Windows functions" (lower case "l" inconsistency in the header).

Text Functions
==============

.. module:: django.db.models.functions.text
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't document the functions in the "text" namespace. Most other references in Django are documented in the convenient import location. Not doing that for functions.datetime was probably a mistake.

@felixxm felixxm changed the title Moved text database functions to a separate file. Refs #28643 -- Reorganized database functions docs and moved text functions to a separate file. Oct 11, 2017
@felixxm
Copy link
Member Author

felixxm commented Oct 11, 2017

@timgraham Thanks for suggestions. I totally agree with you. I added plan to the ticket (see comment) and reorganized existing docs and code.

@timgraham
Copy link
Member

Perhaps the "misc" functions should be documented in their own section rather than at the top of the page at a separate heading level from the categorized functions. I'd make the documentation changes separately so they can be backported to stable/2.0.x, otherwise we're going to have pain backporting any documentation changes to that file in the coming months.

@felixxm felixxm force-pushed the functions-text branch 2 times, most recently from a28c9a1 to f9b9aac Compare October 12, 2017 17:24
@felixxm felixxm changed the title Refs #28643 -- Reorganized database functions docs and moved text functions to a separate file. Refs #28643 -- Reorganized database functions. Oct 12, 2017
@felixxm
Copy link
Member Author

felixxm commented Oct 12, 2017

@timgraham I prepared separate PR with docs changes #9234.

@@ -0,0 +1,85 @@
"""
Classes that represent database functions.
Copy link
Member

Choose a reason for hiding this comment

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

"""Database functions that do comparisons or type conversions."""

Cast, Coalesce, Concat, ConcatPair, Greatest, Least, Length, Lower, Now,
StrIndex, Substr, Upper,
)
from .comp_conv import Cast, Coalesce, Greatest, Least
Copy link
Member

Choose a reason for hiding this comment

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

I think "comparison.py" is fine.


def as_postgresql(self, compiler, connection):
# Postgres' CURRENT_TIMESTAMP means "the time at the start of the
# transaction". We use STATEMENT_TIMESTAMP to be cross-compatible with
Copy link
Member

Choose a reason for hiding this comment

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

While moving this, chop "we".

output_field = fields.DateTimeField()

def as_postgresql(self, compiler, connection):
# Postgres' CURRENT_TIMESTAMP means "the time at the start of the
Copy link
Member

Choose a reason for hiding this comment

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

"Postgres'" looks odd to me. Use "PostgreSQL's" instead.

super().__init__(*expressions, **extra)

def as_oracle(self, compiler, connection):
# we can't mix TextField (NCLOB) and CharField (NVARCHAR), so convert
Copy link
Member

Choose a reason for hiding this comment

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

rewrite without "we": "Oracle prohibits mixing TextField (NCLOB) and CharField (NVARCHAR), so convert all fields to NCLOB when that type is expected."

function = 'TO_NCLOB'

expressions = [
ToNCLOB(expression) for expression in self.get_source_expressions()]
Copy link
Member

Choose a reason for hiding this comment

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

put closing ] on next line

@felixxm felixxm force-pushed the functions-text branch 2 times, most recently from 64331de to 0f6455c Compare October 13, 2017 18:06
Thanks Tim Graham for the review.
@felixxm felixxm merged commit 4f27e47 into django:master Oct 13, 2017
@felixxm felixxm deleted the functions-text branch October 13, 2017 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants