[Django] #23813: Add checks for common URLpattern errors
Django
--------------------------------------+--------------------
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
The system checks framework can be expanded to help less experienced users
help with common mistakes. One source of those is the urlpatterns we have
to configure. It would be nice to have checks in place that verify the
validity and sanity of urls.
One particular example that comes to mind (from my own experience):
{{{
urlpatterns = [
url(r'^my_app/$', include('my_app')),
]
}}}
This would trigger a warning that the trailing '$' in this regular
expression is most likely not intended. :-)
Please do suggest any additional checks that could be added as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/23813>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:1>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
An idea raised in [https://github.com/django/django/pull/3808 pull request
#3808] is to disallow pattern names that contain a colon to avoid
ambiguous namespace lookups.
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:2>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by alasdairnicol):
* cc: alasdair@… (added)
Comment:
In the last few weeks, I've helped a couple of beginners caught out by the
trailing $ when using include. I've had a go at this ticket. I don't think
it's ready to check in yet, so I haven't opened a pull request. I'd
appreciate a code review and any feedback.
https://github.com/alasdairnicol/django/tree/urlchecks
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:3>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
After a quick look your changes look good to me. A PR is much easier to
review though, even if you feel it's not quite ready to be merged. We can
leave feedback on specific lines and also trigger the CI to run its tests.
So please, open up a PR and we'll see if we can get this in on time for
1.9.
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:4>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by alasdairnicol):
Replying to [comment:4 jarshwah]:
> After a quick look your changes look good to me. A PR is much easier to
review though, even if you feel it's not quite ready to be merged. We can
leave feedback on specific lines and also trigger the CI to run its tests.
So please, open up a PR and we'll see if we can get this in on time for
1.9.
Thanks, I've opened a pull request
https://github.com/django/django/pull/5301. In particular, I thought maybe
the warning messages and documentation might need improving.
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:5>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by jarshwah):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
There are a couple of failing tests. See here: http://djangoci.com/job
/pull-requests-trusty/3493/
You'll also need to add some release notes.
I've added a couple of quick comments on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:6>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by alasdairnicol):
Replying to [comment:6 jarshwah]:
> There are a couple of failing tests. See here: http://djangoci.com/job
/pull-requests-trusty/3493/
>
> You'll also need to add some release notes.
>
> I've added a couple of quick comments on the PR.
I've updated the docs as suggested, and added a sentence to the 1.9
release notes.
I tried to look at the failing tests, but I was a bit confused, maybe
because I'm unfamiliar with Jenkins. The failing tests seemed to be
testing the cache. I wasn't expecting these changes to break those tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:7>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jarshwah):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:8>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Sorry about those old test failures, that was a transient issue while I
was making some updates to Jenkins.
As noted on the pull request, the current warnings don't show which url
triggered the warning. I don't think it's acceptable to make the user hunt
their entire project to track down the cause of the warning. :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:9>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by alasdairnicol):
Replying to [comment:9 timgraham]:
> As noted on the pull request, the current warnings don't show which url
triggered the warning. I don't think it's acceptable to make the user hunt
their entire project to track down the cause of the warning. :-)
I've updated the pull request. The warning message now includes the regex
(and name if it has one) of the URL pattern which triggered the warning.
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:10>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: closed
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Josh Smeaton <josh.smeaton@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"fe3fc5210f0bb334a679ed420152af1c862c0239" fe3fc52]:
{{{
#!CommitTicketReference repository=""
revision="fe3fc5210f0bb334a679ed420152af1c862c0239"
Fixed #23813 -- Added checks for common URL pattern errors
Thanks jwa and lamby for the suggestions, and timgraham and jarshwah
for their reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:11>
Django
Reporter: jwa | Owner: jwa
Type: New feature | Status: closed
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"f2975c021d247bf8c6a5fc23988639c636da86f5" f2975c0]:
{{{
#!CommitTicketReference repository=""
revision="f2975c021d247bf8c6a5fc23988639c636da86f5"
Refs #23813 -- Moved URLconfs into module and tidied docstrings.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23813#comment:12>