Fix rule inheritance issue #675

Closed
wants to merge 7 commits into
from

Conversation

2 participants
@tswicegood
Contributor

tswicegood commented Feb 7, 2015

See commit logs for full explanation.

tswicegood added some commits Feb 7, 2015

Add failing test case to show broken Rule issue in 0.10
This was introduced in 426b985 which
properly instantiates the subclass of a Rule instead of returning a Rule
object.  The issue is that empty() uses args instead of kwargs, so
anyone relying on kwargs to inject a new value will have bugs
introduced.
Fix issue where subclasses provide custom kwargs
This makes the previous test pass and makes it unnecessary to do
`kwarg.pop()` work arounds like the one in [this commit][1].

[1]: tswicegood/steinie@2b5c01c
Refactor to allow providing custom kwargs
As noted in the previous commit, subclasses may need to provide their
own kwargs to interact with their custom functionality.  Currently, you
have to overwrite empty() and then add your custom behavior on top of
the default return.  This refactoring allows you to provide your own
custom kwargs by overriding get_empty_kwargs() and modifing the
dictionary of values it returns.
+ class CustomRule(r.Rule):
+ def __init__(self, string=None, custom=None, *args, **kwargs):
+ self.custom = custom
+ super(CustomRule, self).__init__(string, *args, **kwargs)

This comment has been minimized.

@tswicegood

tswicegood Feb 7, 2015

Contributor

This test case could also be modified to remove *args. I'm ambivalent.

@tswicegood

tswicegood Feb 7, 2015

Contributor

This test case could also be modified to remove *args. I'm ambivalent.

tswicegood referenced this pull request Feb 7, 2015

Sharoon Thomas
Fix inheritance with Rule.empty
Rule.empty() always returned an instance of werkzeug.routing.Rule
even when a Rule subclass existed. This patch ensures that the
new instance of the rule returned by empty() is always an instance
of the class the rule is an instance of.
@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Feb 7, 2015

Member

Strictly speaking you're breaking the API of the Rule class if you add your custom arguments to __init__ at any other place than at the very end... so I'd argue that this can't really be called a bug in Werkzeug (yeah, you didn't call it that either, I know).

That said, I strongly agree that explicitly using kwargs is almost always a better solution, we have many places in Werkzeug where large series of positional arguments have caused massive headache. So I'm fine with this fix in principle.

You provided very detailed explanations of the behavior you're introducing, however, I think these explanations have a better place in the code itself. Particularly the reasoning why it is useful that get_empty_kwargs is its own method might become lost over time in the commit log, I think it's better preserved in the method's docstring.

Also, please add a changelog, both for the bugfix and the new overridable method.

This is the kind of PR I'd like to see more often. Thanks.

Member

untitaker commented Feb 7, 2015

Strictly speaking you're breaking the API of the Rule class if you add your custom arguments to __init__ at any other place than at the very end... so I'd argue that this can't really be called a bug in Werkzeug (yeah, you didn't call it that either, I know).

That said, I strongly agree that explicitly using kwargs is almost always a better solution, we have many places in Werkzeug where large series of positional arguments have caused massive headache. So I'm fine with this fix in principle.

You provided very detailed explanations of the behavior you're introducing, however, I think these explanations have a better place in the code itself. Particularly the reasoning why it is useful that get_empty_kwargs is its own method might become lost over time in the commit log, I think it's better preserved in the method's docstring.

Also, please add a changelog, both for the bugfix and the new overridable method.

This is the kind of PR I'd like to see more often. Thanks.

@tswicegood

This comment has been minimized.

Show comment
Hide comment
@tswicegood

tswicegood Feb 8, 2015

Contributor

@untitaker yup -- definitely not a bug in Werkzeug, but the change did cause my code to break because I was relying on the old implementation which didn't instantiate the subclass which, likewise, you can argue wasn't a bug, but it's probably lead to weird issues. :-)

I've updated the changes file and simply added this to the 0.11 version. Not sure if you want to release this as a 0.10.x release or not. Because I wasn't sure on the release, I didn't add a :versionadded: tag to the docstring either. Documentation has been fleshed out for each, however.


Looks like TravisCI just failed, but for unrelated reasons (all of the test_routing tests pass). I'm having issues running the test_server tests locally on Python 3.4, so there might be some issues there.

Contributor

tswicegood commented Feb 8, 2015

@untitaker yup -- definitely not a bug in Werkzeug, but the change did cause my code to break because I was relying on the old implementation which didn't instantiate the subclass which, likewise, you can argue wasn't a bug, but it's probably lead to weird issues. :-)

I've updated the changes file and simply added this to the 0.11 version. Not sure if you want to release this as a 0.10.x release or not. Because I wasn't sure on the release, I didn't add a :versionadded: tag to the docstring either. Documentation has been fleshed out for each, however.


Looks like TravisCI just failed, but for unrelated reasons (all of the test_routing tests pass). I'm having issues running the test_server tests locally on Python 3.4, so there might be some issues there.

Fix this test case so it proves something, not simply the absence
Was thinking about this and it bugged me that this test didn't do
anything positive to ensure it passed.  A silently passing test with no
explicit failure test case will be overlooked later and possibly
removed.  This provides an explicit state that shows the failure along
with a sanity check for empty().
@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Feb 8, 2015

This mangles the traceback though... I'd just let the typeerror bubble up and not worry whether this would become a failure or error.

This mangles the traceback though... I'd just let the typeerror bubble up and not worry whether this would become a failure or error.

This comment has been minimized.

Show comment
Hide comment
@tswicegood

tswicegood Feb 8, 2015

Owner

For me the error vs failure issue wasn't the case, I just wanted to make sure that the test captured the incorrect behavior. I've modified it in 71d14fc to simply re-raise. That allows the exception to bubble up but still provides some insight via the code as to what's not expected to happen.

Owner

tswicegood replied Feb 8, 2015

For me the error vs failure issue wasn't the case, I just wanted to make sure that the test captured the incorrect behavior. I've modified it in 71d14fc to simply re-raise. That allows the exception to bubble up but still provides some insight via the code as to what's not expected to happen.

@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Feb 8, 2015

Member

Don't worry about Travis, test_serving sometimes works, sometimes not.

Member

untitaker commented Feb 8, 2015

Don't worry about Travis, test_serving sometimes works, sometimes not.

@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Feb 8, 2015

Member

I've updated the changes file and simply added this to the 0.11 version. Not sure if you want to release this as a 0.10.x release or not.

Yeah, I think it makes sense to rebase this against the 0.10-maintenance branch. Please file a new pull request against that branch.

Member

untitaker commented Feb 8, 2015

I've updated the changes file and simply added this to the 0.11 version. Not sure if you want to release this as a 0.10.x release or not.

Yeah, I think it makes sense to rebase this against the 0.10-maintenance branch. Please file a new pull request against that branch.

@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Feb 8, 2015

This mangles the traceback though... I'd just let the typeerror bubble up and not worry whether this would become a failure or error.

This mangles the traceback though... I'd just let the typeerror bubble up and not worry whether this would become a failure or error.

This comment has been minimized.

Show comment
Hide comment
@tswicegood

tswicegood Feb 8, 2015

Owner

For me the error vs failure issue wasn't the case, I just wanted to make sure that the test captured the incorrect behavior. I've modified it in 71d14fc to simply re-raise. That allows the exception to bubble up but still provides some insight via the code as to what's not expected to happen.

Owner

tswicegood replied Feb 8, 2015

For me the error vs failure issue wasn't the case, I just wanted to make sure that the test captured the incorrect behavior. I've modified it in 71d14fc to simply re-raise. That allows the exception to bubble up but still provides some insight via the code as to what's not expected to happen.

@tswicegood

This comment has been minimized.

Show comment
Hide comment
@tswicegood

tswicegood Feb 8, 2015

Contributor

Closing to move discussion to #677.

Contributor

tswicegood commented Feb 8, 2015

Closing to move discussion to #677.

@tswicegood tswicegood closed this Feb 8, 2015

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