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

Change aliasing method names in mapper and deprecate the old #746

Merged
merged 3 commits into from Mar 23, 2021

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Mar 19, 2021

Closes #667.

@ml-evs ml-evs force-pushed the ml-evs/close_#667 branch 2 times, most recently from e41644d to b0e2949 Compare March 19, 2021 19:39
@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #746 (db0231d) into master (6152573) will increase coverage by 0.01%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
+ Coverage   92.77%   92.79%   +0.01%     
==========================================
  Files          68       68              
  Lines        3655     3663       +8     
==========================================
+ Hits         3391     3399       +8     
  Misses        264      264              
Flag Coverage Δ
project 92.79% <95.83%> (+0.01%) ⬆️
validator 92.79% <95.83%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/filtertransformers/mongo.py 97.57% <80.00%> (ø)
optimade/server/entry_collections/elasticsearch.py 97.91% <100.00%> (ø)
...made/server/entry_collections/entry_collections.py 97.41% <100.00%> (ø)
optimade/server/mappers/entries.py 98.52% <100.00%> (+0.19%) ⬆️

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 6152573...db0231d. Read the comment docs.

field: Field name to be de-aliased.

Returns:
De-aliased field name, falling back to returning `field`.

"""
field = field.split(".")[0]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this line from the "new" method as it is not tested anywhere and therefore should not be relied on for anything; I think it may just be left over from when alias_for was modified to make this method. If you ask for the OPTIMADE field of e.g. my_weird_field.with_dots, you would want it to return my_weird_field.with_dots rather than alias_for_my_weird_field even if the dot has some significance for that particular backend.

@ml-evs
Copy link
Member Author

ml-evs commented Mar 19, 2021

Is this what you had in mind @CasperWA?

@ml-evs ml-evs marked this pull request as ready for review March 19, 2021 19:57
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the doc string warnings with mkdocs serve and it didn't show correctly. I think the additional explamation point should do it?

Added a sentence about the dot-splitting of get_backend_field() as well, feel free to adapt it as you see fit.

optimade/server/mappers/entries.py Outdated Show resolved Hide resolved
optimade/server/mappers/entries.py Show resolved Hide resolved
optimade/server/mappers/entries.py Outdated Show resolved Hide resolved
optimade/server/mappers/entries.py Outdated Show resolved Hide resolved
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some testing locally, this seems to fix the admonition in the documentation (edited the previous suggested changes to match).

optimade/server/mappers/entries.py Outdated Show resolved Hide resolved
optimade/server/mappers/entries.py Outdated Show resolved Hide resolved
ml-evs and others added 2 commits March 23, 2021 16:43
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@ml-evs
Copy link
Member Author

ml-evs commented Mar 23, 2021

Thanks @CasperWA; I've just noticed that our CI doesn't do a proper rebuild of the docs for pull requests. We should probably fix this, as the elasticsearch docs aren't currently being tested. I'll raise an issue

@ml-evs ml-evs requested a review from CasperWA March 23, 2021 16:49
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers @ml-evs

@CasperWA CasperWA merged commit 4f503eb into master Mar 23, 2021
@CasperWA CasperWA deleted the ml-evs/close_#667 branch March 23, 2021 17:59
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.

Mapper method alias_of extracts alias wrongly
2 participants