Skip to content
This repository has been archived by the owner on Jan 11, 2021. It is now read-only.

A less naive strategy for getting view closure var #306

Merged
merged 2 commits into from Jun 25, 2015

Conversation

mverteuil
Copy link
Contributor

  • Handles a particular class of decorated view functions that refer to the outermost closure arguments.
  • Searches for a types.FunctionType argument in the closure, rather than assuming the first
    argument is the view. This is a correct implementation for existing cases as well.

@ariovistus
Copy link
Contributor

what's your use case that the current implementation doesn't cover?

@mverteuil
Copy link
Contributor Author

I have decorated views that are not being unwrapped correctly, and thus not documented, because 0th index is not a sufficient condition in all cases.

That the view argument is of FunctionType is necessarily true, and provides some flexibility. That flexibility means the difference between whether DRS actually documents the majority of my views, instead of ignoring them because of something trivial.

@ariovistus
Copy link
Contributor

Could you post an example view? I'm curious, and also I want something in the test suite for this.

@mverteuil
Copy link
Contributor Author

Sure. I was trying to craft a test for this on my lunch break yesterday, but the tests file is quite the behemoth and is light on the intent comments and docstrings. I will take another stab at it.

@mverteuil mverteuil force-pushed the less_naive_closure_var branch 2 times, most recently from 6532413 to cc34c08 Compare June 23, 2015 21:27
@mverteuil
Copy link
Contributor Author

I had assumed the bug was larger than it was. However, I've managed to isolate it.

I copied the ViewSets which had issues, then excised until only the code required to exercise the bug remained.

@mverteuil mverteuil force-pushed the less_naive_closure_var branch 3 times, most recently from cc34c08 to 763711b Compare June 23, 2015 21:40
@ariovistus
Copy link
Contributor

sweetness!
linter doesn't like you (I've noticed github doesn't seem to notify the pull requester when travis fails, so just making sure you're aware)

@mverteuil
Copy link
Contributor Author

I keep getting dinged on my amended commits because I'm doing that after running tox. One sec.

@mverteuil
Copy link
Contributor Author

Pushed a linter friendly version of decorators.py

@mverteuil
Copy link
Contributor Author

Working on the test failure now.

@mverteuil
Copy link
Contributor Author

Fixed. I wasn't using the 2/3 compatible form of im_func.

@ariovistus
Copy link
Contributor

your for more info link in the decorator doc doesn't seem to go anywhere?

@mverteuil
Copy link
Contributor Author

I see, it looks like the original branch is now gone, but I don't have a SHA to point to it.

I've been using a version that was proposed in a Django ticket for years now, that code essentially comes from this patch: https://code.djangoproject.com/attachment/ticket/14512/ticket14512.diff

Usually this decorator wraps other decorator invocations that are meant for decorating an FBV. I've simply unwrapped the wrapped decorator and that's where the name 'squashed' comes from. 'Unwrapped' may have been a better choice, but I didn't want to use that term because it has a slightly different (albeit similar) meaning in decorators.py.

I've found a shorter version, here, which isn't EXACTLY the same thing but seems to be for my purposes:
http://stackoverflow.com/questions/6069070/how-to-use-permission-required-decorators-on-django-class-based-views

The tricky thing here is, the shorter version does not seem to exercise the bug because of how it's constructed. Since the scoping is a bit different when method_decorator is used for the inner decoration, it doesn't seem to mess with the freevars. In any event, I still feel like relying on the coincidence of argument position is more fragile and less correct than relying on the fact that the view callable must necessarily be an instance that is FunctionType. While FunctionType type is still a coincidental detail, it has more of a chance to be right. The first argument will then need to also be a FunctionType, and so any time the previous predicate was a true positive it will also be so in this case. Additionally where it was a false positive, it will no longer be one, and where it was a false negative at least there's now an opportunity to find the view callable in other positions.

 - Now handles views decorated with functions that have positional arguments
 - Searches for a types.FunctionType argument in the closure, rather than assuming the first
   argument is the view.
ariovistus added a commit that referenced this pull request Jun 25, 2015
A less naive strategy for getting view closure var
@ariovistus ariovistus merged commit e917f70 into marcgibbons:master Jun 25, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants