-
Notifications
You must be signed in to change notification settings - Fork 802
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
Autogenerate Query classes #1890
Autogenerate Query classes #1890
Conversation
ef8d929
to
a0244fe
Compare
d754326
to
64a2174
Compare
8165e10
to
6660e13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a first review after spending one hour scratching my head on this pull request.
While the inline comments help a lot, there are many things I don't understand yet. We're going to need the review of someone that maintains a typed client as well at some point. @JoshMock and @Anaethelion come to mind, as they have types in their client and are familiar with Python.
aca1d1b
to
9f3977c
Compare
@pquentin thanks for all the feedback. I have addressed all your points I think, and made changes for each in a separate commit so that you can inspect them separately. In addition to your feedback, I also added inherited attributes to all classes, which as I mentioned yesterday I forgot to include before. This actually made evident another thing I needed to handle, the Finally, I've been thinking that the name interfaces.py is odd for Python. Would renaming it to types.py be better? And also, do we want to dump all the queries and types in the root level of the package so that they can be imported directly via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's another round of review. I'm slowly getting to understand this better. Apologies for the reviewing delays, but this is quite daunting!
Finally, I've been thinking that the name interfaces.py is odd for Python. Would renaming it to types.py be better?
Yes, types
is better than interfaces
.
And also, do we want to dump all the queries and types in the root level of the package so that they can be imported directly via from elasticsearch_dsl import X? I recall you indicated you would prefer this way of working with these classes so let me know what you think.
Yes, I think that would fit the existing design better to dump all classes at the top-level. This will address one issue I have with the split between interfaces/types and queries: I still believe it should be an implementation detail that users don't have to care about.
5eb5106
to
1c046aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
bc06d4d
to
efc3c88
Compare
efc3c88
to
0a17b26
Compare
* Autogenerate Query classes * handle Python reserved keywords such as from * replace long deprecated 'filtered' query from tests * minor code generation updates * more code generation updates * address typing issues * clean up code generation templates * more code generator cleanup * add a unit test using some of the generated classes * no need to "reset" the interface list * use the transport's DEFAULT type * include inherited properties in docstrings and constructors * support legacy FieldValueFactor name for FieldValueFactorScore * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * use the identity operator to check for defaults * rename interfaces.py to types.py * leave undocumented classes and attributes with an empty docstring * add unit test for AttrDict with from reserved keyword * add a dependency on the transport library * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * list required arguments first in types.py * add server defaults to argument docstrings * remove unnecessary quotes from type hints in types.py * Update utils/templates/types.py.tpl Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * final round of review improvements --------- Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> (cherry picked from commit bacfc74)
* Autogenerate Query classes * handle Python reserved keywords such as from * replace long deprecated 'filtered' query from tests * minor code generation updates * more code generation updates * address typing issues * clean up code generation templates * more code generator cleanup * add a unit test using some of the generated classes * no need to "reset" the interface list * use the transport's DEFAULT type * include inherited properties in docstrings and constructors * support legacy FieldValueFactor name for FieldValueFactorScore * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * use the identity operator to check for defaults * rename interfaces.py to types.py * leave undocumented classes and attributes with an empty docstring * add unit test for AttrDict with from reserved keyword * add a dependency on the transport library * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * list required arguments first in types.py * add server defaults to argument docstrings * remove unnecessary quotes from type hints in types.py * Update utils/templates/types.py.tpl Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * final round of review improvements --------- Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> (cherry picked from commit bacfc74) Co-authored-by: Miguel Grinberg <miguel.grinberg@gmail.com>
* Autogenerate Query classes * handle Python reserved keywords such as from * replace long deprecated 'filtered' query from tests * minor code generation updates * more code generation updates * address typing issues * clean up code generation templates * more code generator cleanup * add a unit test using some of the generated classes * no need to "reset" the interface list * use the transport's DEFAULT type * include inherited properties in docstrings and constructors * support legacy FieldValueFactor name for FieldValueFactorScore * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * use the identity operator to check for defaults * rename interfaces.py to types.py * leave undocumented classes and attributes with an empty docstring * add unit test for AttrDict with from reserved keyword * add a dependency on the transport library * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * list required arguments first in types.py * add server defaults to argument docstrings * remove unnecessary quotes from type hints in types.py * Update utils/templates/types.py.tpl Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * Update utils/generator.py Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com> * final round of review improvements --------- Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
This change adds a code-generation phase to this client. In this first phase, code generation is applied to two files:
Query
are auto-generated and include docstrings, type hints and a constructor with explicit arguments. All classes havekwargs
that are backwards compatible.Query
sub-classes are defined.Things that are not currently implemented but will likely be added later:
Any
type for now.Union[str, Dict[str, Any]]
type for now.