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

Issue #55 - Database Backend Support For Django 1.7 #62

Closed
wants to merge 2 commits into from

Conversation

sethdenner
Copy link

I got a start on this tonight. Not ready for merge yet.

I just took @kavdev's implementation for _decode_child and refactored it into a sub method definition called decode_child and did the same with the _decode_child implementation from master and put it in a second decode_child definition. Currently I'm using @kavdev's for django.VERSION >= (1, 7) and using the deffinition from master in all other cases.

I still need to fingure out the proper way to get the annotation parameter and verify that the tests pass before this gets mereged.

I managed to find the new location for everything except annotations. I tried self.query.get_aggregation(self.connection.alias) but that gave me a recursion error. I'm pretty sure that if an annotation isn't included in the query, it's set to true.
* Refactored legacy _decode_child code from 'master' branch into a decode_child sub (nested) method.
* Made a second conditional decode child method which gets defined if django.VERSION >= (1, 7). This method has @kavdev's implementation for Django 1.7 support.
* Added a way for a person creating a database backend to implement annotation functionality. I don't think this will be supported well by the rest of djangotoolbox but for this part it should help expanding these features in the future if desired. Implementing that functionallity is outside the scope of this pull request/issue.
@sethdenner
Copy link
Author

OK This is ready to be reviewed now. Is this ready for a merge? Normally I would rebase everything into one commit but I wanted to keep @kavdev's initial commit since he did a bulk of the work in figuring out how to query this new API.

@aburgel
Copy link
Member

aburgel commented Apr 18, 2015

Great stuff @sethdenner! Can you update this to follow the existing coding style? Its a bit hard to see what is new code and what is reformatting.

Also, do these test suites still pass?

@sethdenner
Copy link
Author

Would you prefer that I leave the original formatting and to hell with pep-8? Or would adding comments indicating where the code was refactor from be better?

@kavdev
Copy link
Contributor

kavdev commented Apr 18, 2015

There were only a few pep8 errors. In my experience, it's safe to --ignore=E501,E114,E116,E128.

Here's the atomic version of @sethdenner's commit https://github.com/kavdev/djangotoolbox/commit/03cb2429901a4025db2339dcabe4c760586755d2

The tests won't pass with python 3. #60 seems to have a solution, but I haven't taken a deep look at it.

@aburgel
Copy link
Member

aburgel commented May 25, 2015

Closing because this was merged in #63

@aburgel aburgel closed this May 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants