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

feat: Set E element as ElasticsearchQueryBuilder's attributes #75

Merged
merged 1 commit into from
Jul 30, 2022
Merged

feat: Set E element as ElasticsearchQueryBuilder's attributes #75

merged 1 commit into from
Jul 30, 2022

Conversation

qcoumes
Copy link
Contributor

@qcoumes qcoumes commented Jul 23, 2022

This allows to override elements such as EMust, EWord, ..., without
the need of overriding ElasticsearchQueryBuilder's methods.

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

Cool move @qcoumes !

Before merging, could you eventually add a line at the end of docs/source/quick_start.rst to tell it's possible (giving a sensible use case) ?

@alexgarel
Copy link
Member

@qcoumes; I just merged PR #71 which adds a new operator, you might want to add it before merging.

@alexgarel
Copy link
Member

BTW: please use "Squash and merge" :-)

This allows to override elements such as EMust, EWord, ..., without
the need of overriding ElasticsearchQueryBuilder's methods.
@qcoumes qcoumes requested a review from alexgarel July 30, 2022 00:14
@qcoumes
Copy link
Contributor Author

qcoumes commented Jul 30, 2022

I did not notice you merged the new operator directly onto this branch, so I rebased master and added it from there. Sorry for the force-push.

I added the new element and updated the documentation with a new sub-section Modifying the generated queries under the section Transforming to elastic query.

I don't usually write .rst and have nothing to test locally, I hope I did not make any syntax errors.

I can't select squash and merge ("Only those with write access to this repository can merge pull requests."), but there is only one commit after the force push anyway.

@alexgarel
Copy link
Member

Really cool @qcoumes. Thank you.

No pb for rebasing and force pushing :-)

@alexgarel alexgarel merged commit 5951e61 into jurismarches:master Jul 30, 2022
@alexgarel
Copy link
Member

BTW a simple way to test documentation rendering was simply to visit the file on you branch : https://github.com/qcoumes/luqum/blob/feat/elasticsearch_visitor_element_override/docs/source/quick_start.rst :-)

@qcoumes qcoumes deleted the feat/elasticsearch_visitor_element_override branch August 2, 2022 20:39
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