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

accept int as sort keys instead of Sort() in .query() and .groupby() #241

Merged
merged 6 commits into from
Aug 11, 2020

Conversation

rkingsbury
Copy link
Collaborator

@rkingsbury rkingsbury commented Aug 5, 2020

Accept str or int (e.g. "Ascending" or 1) as sort keys instead of requiring Sort(1) in Store.query(), per previous discussion on Slack.

@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #241 into master will decrease coverage by 0.64%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
- Coverage   88.33%   87.68%   -0.65%     
==========================================
  Files          27       27              
  Lines        2006     2006              
==========================================
- Hits         1772     1759      -13     
- Misses        234      247      +13     
Impacted Files Coverage Δ
src/maggma/core/store.py 88.04% <ø> (ø)
src/maggma/stores/advanced_stores.py 77.90% <ø> (ø)
src/maggma/stores/aws.py 80.92% <ø> (ø)
src/maggma/stores/compound_stores.py 80.89% <ø> (ø)
src/maggma/stores/gridfs.py 89.39% <ø> (ø)
src/maggma/stores/mongolike.py 87.25% <100.00%> (-6.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed4951...9a242a0. Read the comment docs.

@rkingsbury rkingsbury changed the title accept str or int as sort keys instead of Sort() in .query() [WIP] accept str or int as sort keys instead of Sort() in .query() Aug 5, 2020
@rkingsbury rkingsbury changed the title [WIP] accept str or int as sort keys instead of Sort() in .query() accept int as sort keys instead of Sort() in .query() and .groupby() Aug 6, 2020
@lgtm-com
Copy link

lgtm-com bot commented Aug 6, 2020

This pull request introduces 4 alerts when merging 8ba1356 into a6e8e11 - view on LGTM.com

new alerts:

  • 4 for Unused import

@shyamd
Copy link
Contributor

shyamd commented Aug 9, 2020

My only comment is that I wouldn't get rid of the Sort enum which is designed to provide some disambiguation. Rather if the sort is provided as an int then it should be type-checked against sort to make sure it's a valid number and then used. This could be just converted it to a Sort enum.

The type hint in this situation would change from Sort to Union[Sort,int]

@rkingsbury
Copy link
Collaborator Author

@shyamd the docs test is failing due to a version conflict with pymdown-extensions. Is that something I can address?

@shyamd
Copy link
Contributor

shyamd commented Aug 11, 2020

if you merge in the main repo master it should work now.

@shyamd shyamd merged commit e52c8c7 into materialsproject:master Aug 11, 2020
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.

2 participants