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

query args order #31

Closed
montoyjh opened this issue Aug 3, 2018 · 4 comments
Closed

query args order #31

montoyjh opened this issue Aug 3, 2018 · 4 comments
Assignees

Comments

@montoyjh
Copy link
Contributor

montoyjh commented Aug 3, 2018

I know that we've discussed this at least once before, but I think we should reconsider the argument order for criteria and properties for Store.query and Store.query_one. I think criteria should be first and properties should be second. As far as I can tell, the main reason we have properties first is because QueryEngine used to, and as far as I can tell no one who is building dbs for MP using maggma has much experience with or love for QueryEngine.

Basically, I want to be able to teach people how to use maggma by appealing to their knowledge of the MPRester query (or if they've got experience with pymongo, pymongo.Collections.find) by saying "this is basically the same as those things", rather than "this is the same as those things, except the argument order is reversed to keep in line with some other thing we used a long time ago".

@shyamd
Copy link
Contributor

shyamd commented Aug 3, 2018

I'm fine with it. I definitely templated on QueryEngine and had no logical reason for the order. I personally try and use named args whenever I use it so that it doesn't matter.

@montoyjh montoyjh self-assigned this Aug 3, 2018
@montoyjh
Copy link
Contributor Author

montoyjh commented Aug 3, 2018

Cool, I'll submit a PR soon.

@dwinston
Copy link
Member

dwinston commented Aug 3, 2018

Great. I think we should switch. I personally use named args every time only because I want to put criteria first. :) I might get time for this after pre-workshop stuff, but if anyone wants to tackle this with a PR, self-assign this issue. I'll de-assign @shyamd now.

@dwinston
Copy link
Member

dwinston commented Aug 3, 2018

well played, @montoyjh... I should have refreshed the page before submitting that comment. :)

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

No branches or pull requests

3 participants