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

Support version field in SearchClient.post_search() #1079

Merged
merged 11 commits into from
Oct 16, 2024

Conversation

m1yag1
Copy link
Contributor

@m1yag1 m1yag1 commented Oct 11, 2024

https://app.shortcut.com/globus/story/35200/python-sdk-support-version-field-in-searchclient-post-search

Summary of changes

  • Added ignore for SearchQuery in pyproject.toml filterwarnings for pytest
  • Updated SphinxDocs for SearchQueryV1
  • Added SearchQueryV1 to the _generate_init.py script
  • Added SearchQueryV1 class for new v1 search request version
  • Added deprecation warning to the SearchQuery class during init
  • Added unit and functional tests for SearchQueryV1 and SearchQuery deprecation warnings.

📚 Documentation preview 📚: https://globus-sdk-python--1079.org.readthedocs.build/en/1079/

* Added ignore for `SearchQuery` in pyproject.toml filterwarnings for pytest
* Updated SphinxDocs for `SearchQueryV1`
* Added `SearchQueryV1` to the _generate_init.py script
* Added `SearchQueryV1` class for new v1 search request version
* Added deprecation warning to the `SearchQuery` class during init
* Added unit and functional tests for `SearchQueryV1`

Co-authored-by: MaxTueckeGlobus <[email protected]>
@m1yag1 m1yag1 force-pushed the sc-35200-version-field-post-search branch from 39acb8f to 2c9df56 Compare October 11, 2024 19:07
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/globus_sdk/services/search/data.py Outdated Show resolved Hide resolved
src/globus_sdk/services/search/data.py Outdated Show resolved Hide resolved
src/globus_sdk/services/search/data.py Outdated Show resolved Hide resolved
src/globus_sdk/services/search/data.py Outdated Show resolved Hide resolved
tests/unit/helpers/test_search.py Outdated Show resolved Hide resolved
tests/unit/helpers/test_search.py Show resolved Hide resolved
@m1yag1
Copy link
Contributor Author

m1yag1 commented Oct 11, 2024

@sirosen I believe I've taken care of all the comments except the test updates for pytest.mark.filterwarnings. I'll work on each to figure out the necessary test changes and let you know when I'm done.

Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

Some minor final edits requested, and then we should be good to go!

src/globus_sdk/services/search/data.py Outdated Show resolved Hide resolved
:param advanced: Whether to enable (``True``) or not to enable (``False``) advanced
parsing of query strings. The default of ``False`` is robust and guarantees that
the query will not error with "bad query string" errors
:param additional_fields: additional data to include in the query document
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little surprised that pylint is passing, given that we have some undocumented parameters. Maybe I need to look at its settings for __init__... Regardless, we need param doc for filters, etc. It should be pretty short and easy to add.

Copy link
Member

Choose a reason for hiding this comment

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

pylint catches that? PyCharm notifies me about parameter mismatches in docstrings but I haven't spotted that in other linters.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it's supposed to, under our config:

[tool.pylint]
load-plugins = ["pylint.extensions.docparams"]
accept-no-param-doc = "false"

[tool.pylint."messages control"]
disable = [  # simplified -- we have other disabled rules
    "missing-raises-doc",
]

But I'm guessing that it doesn't understand that the class docstring applies to the init method. Maybe it's configurable and we can fix it.

Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

I just caught a minor issue doing my final pre-approval pass. Sorry for not noticing it sooner.

Also, sort needs to be added to the docstring -- I didn't see it there.

src/globus_sdk/services/search/data.py Outdated Show resolved Hide resolved
src/globus_sdk/services/search/data.py Outdated Show resolved Hide resolved
@m1yag1
Copy link
Contributor Author

m1yag1 commented Oct 16, 2024

@sirosen no worries! I've pushed updates for the last review comment on sort.

@m1yag1 m1yag1 merged commit 8b8233f into main Oct 16, 2024
29 checks passed
@m1yag1 m1yag1 deleted the sc-35200-version-field-post-search branch October 16, 2024 21:14
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.

3 participants