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

New post: Django Q objects with py3 and pytest #7

Merged
merged 3 commits into from May 31, 2017

Conversation

3 participants
@jamescooke
Owner

jamescooke commented May 31, 2017

Fixes #6

Adds new post comparing Q objects in Python 3 and links the two posts.

@jamescooke

This comment has been minimized.

Show comment
Hide comment
@jamescooke

jamescooke May 31, 2017

Owner

@yanneves Can I get a merge?

Owner

jamescooke commented May 31, 2017

@yanneves Can I get a merge?

@yanneves

This comment has been minimized.

Show comment
Hide comment
@yanneves

yanneves May 31, 2017

Nice! .rst is a new syntax to me. Looks good to merge.

yanneves commented May 31, 2017

Nice! .rst is a new syntax to me. Looks good to merge.

* `left` and `right` are not considered equal.
"""
assert type(left) is Q
assert type(right) is Q

This comment has been minimized.

@adamchainz
@adamchainz

This comment has been minimized.

@jamescooke

jamescooke May 31, 2017

Owner

I did look at using isinstance and made the decision purely on the output that pytest gives:

================================= FAILURES =================================
________________________________ test_fails ________________________________
test_assert_q_equal.py:112: in test_fails
    assert isinstance(a, Q)
E   AssertionError: assert False
E    +  where False = isinstance('Q', Q)
_______________________________ test_fails2 ________________________________
test_assert_q_equal.py:116: in test_fails2
    assert type(a) is Q
E   AssertionError: assert <class 'str'> is Q
E    +  where <class 'str'> = type('Q')
==================== 2 failed, 8 passed in 0.07 seconds ====================

Using type means that pytest explains what the type was that was found in the output and I found this more helpful.

Do you think I should abandon that idea and go for isinstance for the flexibility?

@jamescooke

jamescooke May 31, 2017

Owner

I did look at using isinstance and made the decision purely on the output that pytest gives:

================================= FAILURES =================================
________________________________ test_fails ________________________________
test_assert_q_equal.py:112: in test_fails
    assert isinstance(a, Q)
E   AssertionError: assert False
E    +  where False = isinstance('Q', Q)
_______________________________ test_fails2 ________________________________
test_assert_q_equal.py:116: in test_fails2
    assert type(a) is Q
E   AssertionError: assert <class 'str'> is Q
E    +  where <class 'str'> = type('Q')
==================== 2 failed, 8 passed in 0.07 seconds ====================

Using type means that pytest explains what the type was that was found in the output and I found this more helpful.

Do you think I should abandon that idea and go for isinstance for the flexibility?

This comment has been minimized.

@adamchainz

adamchainz May 31, 2017

well you could just assert isinstance(a, Q), f'{a.__class__} is not subclass of Q'

@adamchainz

adamchainz May 31, 2017

well you could just assert isinstance(a, Q), f'{a.__class__} is not subclass of Q'

This comment has been minimized.

@jamescooke

jamescooke Jun 1, 2017

Owner

That's a much better idea than either of mine! I'll try it out and PR it in.

@jamescooke

jamescooke Jun 1, 2017

Owner

That's a much better idea than either of mine! I'll try it out and PR it in.

Django's Q object does not implement ``__cmp__`` and neither does
``Node`` which it extends (``Node`` is in the ``django.utils.tree`` module).
Unfortunately, that means that comparison of Q objects that are equal fails.

This comment has been minimized.

@adamchainz

adamchainz May 31, 2017

you know, django does accept PR's

@adamchainz

adamchainz May 31, 2017

you know, django does accept PR's

This comment has been minimized.

@jamescooke

jamescooke May 31, 2017

Owner

My guess would be that do to this well could take quite a large amount of effort while the payoff would be small. In the original post I wrote about why I thought this would not be implemented in Django: http://jamescooke.info/comparing-django-q-objects.html#the-perfect-world-predicate-logic

What do you think?

@jamescooke

jamescooke May 31, 2017

Owner

My guess would be that do to this well could take quite a large amount of effort while the payoff would be small. In the original post I wrote about why I thought this would not be implemented in Django: http://jamescooke.info/comparing-django-q-objects.html#the-perfect-world-predicate-logic

What do you think?

This comment has been minimized.

@adamchainz

adamchainz May 31, 2017

You could always argue that cmp could be done with str(self) == str(other) as a basic workaround like you have here

@adamchainz

adamchainz May 31, 2017

You could always argue that cmp could be done with str(self) == str(other) as a basic workaround like you have here

This comment has been minimized.

@jamescooke

jamescooke Jun 1, 2017

Owner

That sounds like an idea - I might give it a go 👍

@jamescooke

jamescooke Jun 1, 2017

Owner

That sounds like an idea - I might give it a go 👍

@jamescooke

I went and added my comment as a review like a GitHub n00b.

Django's Q object does not implement ``__cmp__`` and neither does
``Node`` which it extends (``Node`` is in the ``django.utils.tree`` module).
Unfortunately, that means that comparison of Q objects that are equal fails.

This comment has been minimized.

@jamescooke

jamescooke May 31, 2017

Owner

My guess would be that do to this well could take quite a large amount of effort while the payoff would be small. In the original post I wrote about why I thought this would not be implemented in Django: http://jamescooke.info/comparing-django-q-objects.html#the-perfect-world-predicate-logic

What do you think?

@jamescooke

jamescooke May 31, 2017

Owner

My guess would be that do to this well could take quite a large amount of effort while the payoff would be small. In the original post I wrote about why I thought this would not be implemented in Django: http://jamescooke.info/comparing-django-q-objects.html#the-perfect-world-predicate-logic

What do you think?

@jamescooke

This comment has been minimized.

Show comment
Hide comment
@jamescooke

jamescooke May 31, 2017

Owner

@yanneves Thanks for reading.

@adamchainz Thanks for reviewing.

I'm going to merge and publish but I'll update the post based on your comments Adam 😄

Owner

jamescooke commented May 31, 2017

@yanneves Thanks for reading.

@adamchainz Thanks for reviewing.

I'm going to merge and publish but I'll update the post based on your comments Adam 😄

@jamescooke jamescooke merged commit efe7929 into master May 31, 2017

@jamescooke jamescooke deleted the django-q-obj-py-3 branch May 31, 2017

@jamescooke jamescooke referenced this pull request Jun 4, 2017

Merged

Improve Q comparison #8

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