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

Create template filtertransformer BaseTransformer #287

Merged
merged 8 commits into from Dec 7, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jun 2, 2020

Here's a first pass at a BaseTransformer. I think we need to discuss what to do with the Django, JSON and TransformerSkeleton and elasticsearch transformers, plus all the grammar variants... I haven't really gotten my head around how lark interacts with these magic-word methods, and how we currently have different method names between al the transformers.

For now, I've made most of the BaseTransformer methods abstract, apart from those that do very simple things (i.e. take in a value and return value).

  • Add deprecation warning for Lark2Django/DjangoTransformer, which is working from 0.9.7 grammar and will be tricky to adapt
  • Remove JSONTransformer and DebugTransformer (and updated docs)
  • Created abstract BaseTransformer and pulled mongo functionality out
  • Made file for v1.0.0 grammar that links to v0.10.1

@ml-evs ml-evs mentioned this pull request Jun 2, 2020
@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #287 (23a2a80) into master (36af320) will increase coverage by 1.06%.
The diff coverage is 97.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
+ Coverage   92.20%   93.27%   +1.06%     
==========================================
  Files          61       60       -1     
  Lines        3246     3256      +10     
==========================================
+ Hits         2993     3037      +44     
+ Misses        253      219      -34     
Flag Coverage Δ
project 93.27% <97.80%> (+1.06%) ⬆️
validator 66.89% <87.91%> (+0.99%) ⬆️

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

Impacted Files Coverage Δ
optimade/validator/__init__.py 100.00% <ø> (+91.42%) ⬆️
optimade/filtertransformers/base_transformer.py 97.46% <97.46%> (ø)
optimade/filtertransformers/django.py 91.52% <100.00%> (+0.45%) ⬆️
optimade/filtertransformers/elasticsearch.py 87.97% <100.00%> (+0.15%) ⬆️
optimade/filtertransformers/mongo.py 97.36% <100.00%> (ø)
optimade/server/entry_collections/mongo.py 95.31% <100.00%> (ø)

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 36af320...23a2a80. Read the comment docs.

@fekad
Copy link
Contributor

fekad commented Jun 7, 2020

The TransformerSkeleton can be removed for sure. That was just a template for the "developers" and used for debugging.

@ml-evs ml-evs force-pushed the ml-evs/base_transformer branch 2 times, most recently from c920645 to d5d2928 Compare October 6, 2020 00:25
@ml-evs ml-evs force-pushed the ml-evs/base_transformer branch 5 times, most recently from a832c7f to 0703397 Compare November 23, 2020 20:35
@ml-evs ml-evs marked this pull request as ready for review November 24, 2020 02:08
@ml-evs ml-evs added blocking For issues/PRs that are blocking other PRs. transformers Related to all filter transformers priority/medium Issue or PR with a consensus of medium priority labels Nov 24, 2020
@ml-evs ml-evs requested a review from CasperWA November 24, 2020 02:09
- Raise deprecation warning for DjangoTransformer
- Removed debug/json transformers
- Tweaked basetransformer docstrings
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.

Looking good, thanks @ml-evs!

Are there any actual changes in the grammar?

I think "Return as is." is more telling than "Do nothing!" - so I have suggested this change everywhere. There are also some other comments, but only suggestions.

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
optimade/filtertransformers/base_transformer.py Outdated Show resolved Hide resolved
optimade/filtertransformers/base_transformer.py Outdated Show resolved Hide resolved
optimade/filtertransformers/base_transformer.py Outdated Show resolved Hide resolved
optimade/filtertransformers/base_transformer.py Outdated Show resolved Hide resolved
optimade/filtertransformers/django.py Show resolved Hide resolved
optimade/filtertransformers/elasticsearch.py Show resolved Hide resolved
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@ml-evs ml-evs merged commit f5e7b57 into master Dec 7, 2020
@ml-evs ml-evs deleted the ml-evs/base_transformer branch December 7, 2020 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking For issues/PRs that are blocking other PRs. priority/medium Issue or PR with a consensus of medium priority transformers Related to all filter transformers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants