-
Notifications
You must be signed in to change notification settings - Fork 37
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
836 Django 2.0 Upgrade #840
Conversation
Also worth noting: some tests are currently commented out, just so I can reduce the noise. They'll be back operational once I figure out what's up with the serializers. Edit: you shouldn't see any commented-out tests any more. |
@@ -33,50 +33,18 @@ def logout(request): | |||
else: | |||
return render(request, 'logout.html') | |||
|
|||
def handler400(request, exception): | |||
return render(request, '400.html', {}) |
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.
Worth noting: we don't actually have a 400.html, so we're not really testing this properly right now. I'm going to log a separate issue to sort that out.
Codecov Report
@@ Coverage Diff @@
## master #840 +/- ##
==========================================
+ Coverage 90.99% 91.22% +0.22%
==========================================
Files 39 39
Lines 1754 1754
==========================================
+ Hits 1596 1600 +4
+ Misses 158 154 -4
Continue to review full report at Codecov.
|
view=views.UserFormView.as_view(), name='UserFormView'), | ||
path('', UserListView.as_view(), name='UserListView'), | ||
path('<username>/', UserDetailView.as_view(), name='UserDetailView'), | ||
path('e/<username>/', UserFormView.as_view(), name='UserFormView'), | ||
] |
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.
Paths are no longer trying to match a pattern here. Is that no longer necessary? If so, use re_path
would be necessary
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.
Yeah, the new path stuff is pretty interesting, and quite a bit cleaner. Underneath there's still a lot of regex stuff happening, but in urls.py you can ignore all that. You can pass arguments to path, no problem, and it's quite a bit cleaner to do so. In this particular case, we're just capturing username and passing it on to the view. Spiffy!
In my ideal world, I'd limit this to slug matches with slug:username but we allow characters in usernames that aren't allowed in slugs (like periods) so that won't work.
You should only need to dip down into re_path if you've got a full-on regex to match against or -- and I suspect this is the more common case) you've got an older URL that feels messy to match up to the new path style.
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.
I see, I was worrying that we are allowing more additional characters than we needed to. Just want to make sure that would be okay.
But sounds like it may not be a problem.
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.
That's a valid concern. We are leaving username pretty open here, but they should be validated when the user is created, and if an invalid character comes in it'll just 404.
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.
Looks great to me.
Description
Upgrades to Django 2.0.
A few things to note: