Skip to content

Allow dot notation indices of embedded documents #77

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

Closed

Conversation

stillinbeta
Copy link

I mentioned earlier on the mailing list that I wanted to index embedded documents, as Mongo allows, and I was curious as to weather it was possible. It wasn't, so I did my best to fix it.

I made sure to preserve column aliases at the root level, but in the case of embedded models I do not currently check for aliases. Is this an important feature, or is what I've written reasonable?

@jonashaag
Copy link
Contributor

We should also recurse in to embedded models. I can definitely help you with implementation if you're stuck.

@stillinbeta
Copy link
Author

I wasn't stuck, it was just 2am and I was tired :)

@jonashaag
Copy link
Contributor

Gna. I wanted the diff show this: https://github.com/django-mongodb-engine/mongodb-engine/compare/master...issue77

I think that makes the find_field code a bit easier to understand/parse. What do you think?

@stillinbeta
Copy link
Author

I'm not sure how I feel about using is_instance. I used hasattr because I think in Python an "interface" should be determined by having attributes, not by checking superclass. For example, an element is iterable if it has an __iter__ method, not because it subclasses something.

I'm far from a python grandmaster though, and my intuition could be way off. Thoughts?

@jonashaag
Copy link
Contributor

Ellie: It's clearer with isinstance. With hasattr, we'd need to explicitly mention what this attribute is about in a comment.

[OT]Python is duckly typed in theory but (in my opinion) misses some important constructs (like interfaces) to make type checks usable (for example, there's no straightforward duckly way to check for a string/unicode/list/dict etc). That's why nobody uses duck-type-checks in practice. The only exception I know about is hasattr(thing, 'read') --> thing is a file which is so wide-spread that everybody understands.[/OT]

Besides that, I'm not totally happy with how get_column_name is implemented (the replace list items in-place thing bugs me), so I'm still happy to accept improvements.

@flaper87 do you have some time to review my patch?

@stillinbeta
Copy link
Author

Ok, fair enough. I'll pull your commit in and push, I think that's the appropriate workflow?

@jonashaag
Copy link
Contributor

If you want to change anything, yes. Otherwise I can just merge my branch into master.

@stillinbeta
Copy link
Author

My pep8 changes were done after you forked my branch, so I'll need to push with those.

@jonashaag jonashaag closed this in 0126c30 Oct 30, 2011
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