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 #22531 and added tests for tree.Node #2628

Closed
wants to merge 1 commit into from
Closed

Fixed #22531 and added tests for tree.Node #2628

wants to merge 1 commit into from

Conversation

mmardini
Copy link
Contributor

@mmardini mmardini commented May 3, 2014

While Node class has a useful __str__, its __repr__ is not that
useful. Added a __repr__ that makes use of the current __str__.
This is especially useful since the more popular Q class inherits
tree.Node.

@timgraham
Copy link
Member

A test would be good.

@mmardini
Copy link
Contributor Author

mmardini commented May 7, 2014

Thanks Tim for your feedback. After examining the internal tests of Django, I couldn't find a suitable place where a test for this function would really belong. queries/tests.py has tests which use Q objects, but in the context of creating querysets. Would using that file be feasible? Any other recommendation?

@timgraham
Copy link
Member

I would add a new file: tests/utils_tests/test_tree.py

@timgraham
Copy link
Member

Can one of the admins verify this patch?

While Node class has a useful `__str__`, its `__repr__` is not that
useful. Added a `__repr__` that makes use of the current `__str__`.
This is especially useful since the more popular `Q` class inherits
`tree.Node`. Also created new tests that cover most of `Node` class
functionality.
@mmardini mmardini changed the title Fixed #22531 -- Added __repr__ for tree.Node Fixed #22531 and added tests for tree.Node May 16, 2014
@mmardini
Copy link
Contributor Author

Added a new tests file. Please verify it too.

@timgraham
Copy link
Member

Looks good. There were a couple minor flake8 warnings I fixed. merged in 393ddc1.

@timgraham timgraham closed this May 16, 2014
@mmardini
Copy link
Contributor Author

Thanks Tim for the review. I will check using flake8 in my future PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants