Skip to content
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

Fixed get_related_model with hybrid_property.expression #355

Merged
merged 1 commit into from Sep 3, 2014

Conversation

@andir
Copy link
Contributor

@andir andir commented Sep 2, 2014

Previously when using a hybrid_property.expression, the get_related_model() function would throw an exception like:

  [...]
  File "..python3.4/site-packages/flask_restless/views.py", line 1056, in get
    return self._search()
  File "..python3.4/site-packages/flask_restless/views.py", line 999, in _search
    relations = frozenset(get_relations(self.model))
  File "..python3.4/site-packages/flask_restless/helpers.py", line 114, in get_relations
    return [k for k in dir(model)
  File "..python3.4/site-packages/flask_restless/helpers.py", line 116, in <listcomp>
    and get_related_model(model, k)]
  File "..python3.4/site-packages/flask_restless/helpers.py", line 129, in get_related_model
    if isinstance(attr, AssociationProxy):
UnboundLocalError: local variable 'attr' referenced before assignment

I believe the original code was just indented wrong accidentally.

@jfinkels
Copy link
Owner

@jfinkels jfinkels commented Sep 2, 2014

This is the missing test I was asking for in #351. I will merge this later today. Thanks!

Loading

return None
return self.integer > 21

@is_above_21.expression
Copy link
Owner

@jfinkels jfinkels Sep 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just place this function in the Person class? I prefer not to add more models unless absolutely necessary.

Loading

andir added a commit to andir/flask-restless that referenced this issue Sep 2, 2014
@andir
Copy link
Contributor Author

@andir andir commented Sep 2, 2014

I've added it to the person model. I still added it as 2nd hybrid_property since the behaviour without the expression is slightly different and doesn't trigger this issue.

Whats the PR workflow you prefer for these kind of fixes? Squash them together into a single commit?

Loading

@jfinkels
Copy link
Owner

@jfinkels jfinkels commented Sep 2, 2014

Yes, I prefer squashed commits in pull requests. (Doesn't everyone?)

Loading

@andir andir force-pushed the fix_hybrid_property_related branch from a131a74 to fba887f Sep 2, 2014
@andir
Copy link
Contributor Author

@andir andir commented Sep 2, 2014

Ok, squashed and pushed again.

I personally always disliked that the PR history got kind of destroyed
(comments on previous commits where vanished) when pushing (with --force)
after some comments had been made. Seems like Github changed that at some
point in the last ~4 years...

Loading

Previously when using a hyprid_property.expression get_related_model would throw an exception like:

  [..]
  File "..python3.4/site-packages/flask_restless/views.py", line 1056, in get
    return self._search()
  File "..python3.4/site-packages/flask_restless/views.py", line 999, in _search
    relations = frozenset(get_relations(self.model))
  File "..python3.4/site-packages/flask_restless/helpers.py", line 114, in get_relations
    return [k for k in dir(model)
  File "..python3.4/site-packages/flask_restless/helpers.py", line 116, in <listcomp>
    and get_related_model(model, k)]
  File "..python3.4/site-packages/flask_restless/helpers.py", line 129, in get_related_model
    if isinstance(attr, AssociationProxy):
UnboundLocalError: local variable 'attr' referenced before assignment

I belive the original code was just indented wrong accidentially.
@andir andir force-pushed the fix_hybrid_property_related branch from fba887f to f5d2fd1 Sep 2, 2014
jfinkels added a commit that referenced this issue Sep 3, 2014
Fixed get_related_model with hybrid_property.expression
@jfinkels jfinkels merged commit c8d4db9 into jfinkels:master Sep 3, 2014
1 check passed
Loading
@andir andir deleted the fix_hybrid_property_related branch Sep 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants