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

Expose HTTPRedirect default status #1611

Merged
merged 5 commits into from Oct 29, 2017

Conversation

Projects
None yet
4 participants
@jaraco
Member

jaraco commented Jul 12, 2017

While adapting some internal code, I had a situation where I wanted to handle the response directly rather than raising an HTTPRedirect, but I still wanted to use the same response code. After inspecting the logic for how HTTPRedirect calculates the appropriate response code for a redirect, I realized I wanted to re-use this logic rather than copy/paste it.

This change extracts that logic into the HTTPRedirect.default_status class property, such that it can be used by a third party application even if HTTPRedirect itself isn't used.

I'm a little sad this change adds a new dependency for the classproperty, but since it's not stdlib, that's the best option.

jaraco added some commits Jul 12, 2017

@jaraco jaraco added the enhancement label Jul 12, 2017

status = int(status)
if status < 300 or status > 399:
raise ValueError('status must be between 300 and 399.')
status = int(status) if status is not None else self.default_status

This comment has been minimized.

@webknjaz

webknjaz Jul 12, 2017

Member

Why not assign self.status right here?

This comment has been minimized.

@jaraco

jaraco Jul 26, 2017

Member

Yes, I think that would be preferable.

raise ValueError('status must be between 300 and 399.')
status = int(status) if status is not None else self.default_status
if not 300 <= status <= 399:
raise ValueError('status must be between 300 and 399.')
self.status = status
CherryPyException.__init__(self, abs_urls, status)

This comment has been minimized.

@webknjaz

webknjaz Jul 12, 2017

Member

Can we use super()?

This comment has been minimized.

@jaraco

jaraco Jul 26, 2017

Member

Yes, perhaps, but that's outside the scope of this PR.

@@ -123,6 +123,9 @@ class Root:
import six
from six.moves import urllib
from jaraco.classes.properties import classproperty

This comment has been minimized.

@webknjaz

webknjaz Jul 12, 2017

Member

I'd move it before six preserving alphabetic order

This comment has been minimized.

@jaraco

jaraco Jul 26, 2017

Member

I've been using a convention of creating four sections for imports: stdlib, compatibility (stdlib functionality but from third-party modules), third-party modules, and finally intrapackage imports. Perhaps my convention diverges slightly from the more common convention of putting all third-party imports (even six) in the same group.

This comment has been minimized.

@webknjaz

webknjaz Jul 26, 2017

Member

No, it's okay. It's good that you explained your way of structuring imports. I think makes sense, we should probably document this approach in contribution docs to make everyone follow the same code style.

@Safihre

This comment has been minimized.

Contributor

Safihre commented Jul 18, 2017

Another dependency.. And also a non standard language concept of class property.
Is there really no better and more general solution?

self.status = status
CherryPyException.__init__(self, abs_urls, status)
@classproperty
def default_status(cls):

This comment has been minimized.

@jaraco

jaraco Jul 26, 2017

Member

codacy fail - this is a classmethod, so the convention is cls.

self.status = status
CherryPyException.__init__(self, abs_urls, status)
@classproperty
def default_status(cls):

This comment has been minimized.

@jaraco

jaraco Jul 26, 2017

Member

codacy fail - it can't be a static method because it's a class property.

@jaraco

This comment has been minimized.

Member

jaraco commented Jul 26, 2017

Another dependency.. And also a non standard language concept of class property.
Is there really no better and more general solution?

It is a non-standard concept; i.e. it's not in the stdlib. I personally think it should be as it's a fairly basic concept in OO, even if only occasionally used.

Yes, the additional dependency makes me a little sad, but not as sad as a copy/paste of the implementation (and subsequent maintenance burden). That is, if we assume a classproperty is desired.

I believe a classproperty is desired. It would be trivial to make that default_status a static method, such that the usage is HTTPRedirect.default_status() or HTTPRedirect.get_default_status(), but that feels a little too verbose to me. Of course, such a method would have the advantage of communicating to the user that the value is calculated (and might change based on context).

@Safihre : Classproperty details aside, what interface do you think is best for the end user who wishes to resolve the default status for a redirect?

@cherrypy cherrypy deleted a comment from codacy-bot Jul 26, 2017

@cherrypy cherrypy deleted a comment from codacy-bot Jul 26, 2017

@cherrypy cherrypy deleted a comment from codacy-bot Jul 26, 2017

@Safihre

This comment has been minimized.

Contributor

Safihre commented Aug 1, 2017

It would be trivial to make that default_status a static method, such that the usage is HTTPRedirect.default_status() or HTTPRedirect.get_default_status(), but that feels a little too verbose to me. Of course, such a method would have the advantage of communicating to the user that the value is calculated (and might change based on context).

What is so wrong with that?
get_default_status() seems pretty reasonable and understandable if an external third party application wants to get this info: it's a getter/setter-style.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 1, 2017

it's a getter/setter-style

It's normally applied to instances, not classes. It's pythonic way to use @property decorator, but it's not applicable to properties at the class level. I support @jaraco's intention to bring a bit more elegance into the codebase.

@jaraco jaraco force-pushed the feature/http-redirect-default-status branch from df9be29 to c92503e Oct 28, 2017

@cherrypy cherrypy deleted a comment from codacy-bot Oct 28, 2017

@cherrypy cherrypy deleted a comment from codacy-bot Oct 28, 2017

CherryPyException.__init__(self, abs_urls, status)
@classproperty
def default_status(cls):

This comment has been minimized.

@codacy-bot
CherryPyException.__init__(self, abs_urls, status)
@classproperty
def default_status(cls):

This comment has been minimized.

@codacy-bot

This comment has been minimized.

codacy-bot commented Oct 28, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 2
           

See the complete overview on Codacy

@cherrypy cherrypy deleted a comment from codacy-bot Oct 28, 2017

@codecov

This comment has been minimized.

codecov bot commented Oct 29, 2017

Codecov Report

Merging #1611 into master will decrease coverage by 0.44%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1611      +/-   ##
==========================================
- Coverage   77.56%   77.12%   -0.45%     
==========================================
  Files         106      106              
  Lines       14374    14372       -2     
==========================================
- Hits        11149    11084      -65     
- Misses       3225     3288      +63

@jaraco jaraco merged commit 52ab4a1 into master Oct 29, 2017

8 of 9 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 83.33% of diff hit (target 77.56%)
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details

@jaraco jaraco deleted the feature/http-redirect-default-status branch Dec 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment