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

Use Python types to declare document fields #1845

Merged
merged 12 commits into from
Jun 21, 2024

Conversation

miguelgrinberg
Copy link
Collaborator

@miguelgrinberg miguelgrinberg commented Jun 13, 2024

With this change, Document and InnerDoc instances can optionally use Python type hints instead or in addition to DSL field objects, and inherit dataclass-style behaviors which allow type checkers to correctly infer types.

@miguelgrinberg miguelgrinberg requested a review from pquentin June 18, 2024 16:21
@miguelgrinberg miguelgrinberg marked this pull request as ready for review June 18, 2024 16:24
@miguelgrinberg
Copy link
Collaborator Author

@pquentin I suggest you start from the new documentation in persistence.rst when reviewing this.

docs/persistence.rst Outdated Show resolved Hide resolved
Comment on lines +124 to +142
.. list-table:: Python type to DSL field mappings
:header-rows: 1

* - Python type
- DSL field
* - ``str``
- ``Text(required=True)``
* - ``bool``
- ``Boolean(required=True)``
* - ``int``
- ``Integer(required=True)``
* - ``float``
- ``Float(required=True)``
* - ``bytes``
- ``Binary(required=True)``
* - ``datetime``
- ``Date(required=True)``
* - ``date``
- ``Date(format="yyyy-MM-dd", required=True)``
Copy link
Member

Choose a reason for hiding this comment

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

This is very helpful, should we also add List and Optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, I'll add them to the table.

Copy link
Member

Choose a reason for hiding this comment

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

Did you decide against adding Optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't. Sorry, I added List, but left out Optional. I'm adding it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, so I tried a couple of different formats and found that growing the table makes it too long and more difficult to visually parse. So in the end I kept the table small with just the required variants of the Python native types, and right after the table I show examples of Optional, List, Object and Nested. Let me know what you think.

docs/persistence.rst Outdated Show resolved Hide resolved
docs/persistence.rst Outdated Show resolved Hide resolved
docs/persistence.rst Outdated Show resolved Hide resolved
docs/persistence.rst Show resolved Hide resolved
elasticsearch_dsl/document_base.py Outdated Show resolved Hide resolved
elasticsearch_dsl/document_base.py Outdated Show resolved Hide resolved
elasticsearch_dsl/search_base.py Show resolved Hide resolved
@pquentin
Copy link
Member

By the way, I forgot to mention it, but this is very cool 👍

miguelgrinberg and others added 2 commits June 20, 2024 10:13
Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
@miguelgrinberg miguelgrinberg requested a review from pquentin June 20, 2024 14:31
@miguelgrinberg
Copy link
Collaborator Author

@pquentin Thanks for all the feedback! I think I have addressed all of your notes. I rewrote the __getattr__ implementation in InstrumentedField in a way that is much more clear (to me, at least), and I have also added a unit test that explicitly tries all the possible ways this class can be used. Let me know what you think.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! Only left two questions.

Comment on lines +124 to +142
.. list-table:: Python type to DSL field mappings
:header-rows: 1

* - Python type
- DSL field
* - ``str``
- ``Text(required=True)``
* - ``bool``
- ``Boolean(required=True)``
* - ``int``
- ``Integer(required=True)``
* - ``float``
- ``Float(required=True)``
* - ``bytes``
- ``Binary(required=True)``
* - ``datetime``
- ``Date(required=True)``
* - ``date``
- ``Date(format="yyyy-MM-dd", required=True)``
Copy link
Member

Choose a reason for hiding this comment

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

Did you decide against adding Optional?

docs/persistence.rst Outdated Show resolved Hide resolved
@miguelgrinberg miguelgrinberg requested a review from pquentin June 21, 2024 14:18
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, even if the table doesn't have List or Optional. Maybe better to explain it afterwards as you did indeed.

@miguelgrinberg miguelgrinberg added the backport 8.x Backport to 8.x label Jun 21, 2024
@miguelgrinberg miguelgrinberg merged commit 0c3ffcd into elastic:main Jun 21, 2024
17 checks passed
@miguelgrinberg miguelgrinberg deleted the typing-hints branch June 21, 2024 14:59
github-actions bot pushed a commit that referenced this pull request Jun 21, 2024
* basic type annotation support

* support for optional, list, and other type hints

* additional typing support

* dataclass-like behavior for Document and InnerDoc

* unit tests

* support InstrumentedField in Search class

* documentation

* Update docs/persistence.rst

Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>

* Update elasticsearch_dsl/document_base.py

Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>

* addressed review feedback

* better docs for Optional

* fix optional in test

---------

Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
(cherry picked from commit 0c3ffcd)
miguelgrinberg added a commit that referenced this pull request Jun 21, 2024
* basic type annotation support

* support for optional, list, and other type hints

* additional typing support

* dataclass-like behavior for Document and InnerDoc

* unit tests

* support InstrumentedField in Search class

* documentation

* Update docs/persistence.rst

Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>

* Update elasticsearch_dsl/document_base.py

Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>

* addressed review feedback

* better docs for Optional

* fix optional in test

---------

Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
(cherry picked from commit 0c3ffcd)

Co-authored-by: Miguel Grinberg <miguel.grinberg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 8.x Backport to 8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants