--
Ticket URL: <https://code.djangoproject.com/ticket/22064>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* needs_better_patch: => 0
* needs_docs: => 0
* type: Uncategorized => Cleanup/optimization
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Good idea, that sounds like a perfect use-case for the checks framework.
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:1>
* version: 1.6 => master
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:2>
Comment (by mondone):
https://github.com/mondone/django/tree/ticket_22064
work in progress (tests)
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:3>
* owner: mondone => elhippie
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:4>
Comment (by Akshay Katyal <ktyaks@…>):
Is anyone working on this? Been about 3 months since the last commit on
the branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:5>
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 1 => 0
Comment:
The commit in comment 3 looks like a good start. There's a comment there
that says, "There is a problem with regex performance, it must be solved
before pull request" so if someone wants to take a look, that would be
helpful.
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:6>
Comment (by aericson):
Curious if the problem with regex is the fact that the commit uses two
regexes or is it that he uses regex at all?
I mean would it be preferable to have a faster less readable regex-free
solution or a slower more readable using regex?
Please read this as a general question from someone new here rather than
as specific to this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:7>
Comment (by carljm):
The checks framework is not a runtime code path or a fast path at all, so
readability definitely trumps performance here. Barring catastrophic
performance issues, which a complex regex can be susceptible to.
Looking at the draft patch commit, a few comments:
1. I don't see why the "ends with +" case needs any regex at all. Any
related-name ending with "+" is valid, since it won't be used as a Python
identifier anyway. (I'm not sure why we even support "ends with +" rather
than just "+" -- what's the purpose of including anything before the
"+"?). So a simple `.endswith('+')` check should be sufficient there.
2. I'd suggest that we also check the `related_name` against
`keyword.iskeyword()`, since using a Python keyword as a related name
could also cause trouble.
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:8>
Comment (by aericson):
Thanks for your comments I'll be submitting a patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:9>
* owner: elhippie => aericson
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:10>
Comment (by aericson):
Assigned it to myself as it seems to be inactive for months.
Submitted a new patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:11>
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1e5e2a470768549117ac4696ce3e8b4e51d65664"]:
{{{
#!CommitTicketReference repository=""
revision="1e5e2a470768549117ac4696ce3e8b4e51d65664"
Fixed #22064 -- Add check for related_name
Validates that related_name is a valid Python id or ends with a '+' and
it's not a keyword. Without a check it passed silently leading to
unpredictable problems.
Thanks Konrad Świat for the initial work.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:12>
Comment (by Carl Meyer <carl@…>):
In [changeset:"6f6e7d01dce94668e178b26da547c4643ed3a6cc"]:
{{{
#!CommitTicketReference repository=""
revision="6f6e7d01dce94668e178b26da547c4643ed3a6cc"
Merge pull request #3308 from aericson/ticket_22064
Fixed #22064 -- Add check for related_name
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22064#comment:13>