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

Update to use the v2 API as v1 has been fully discontinued #6

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ncoish
Copy link

@ncoish ncoish commented Jul 13, 2022

As of today (2022-07-13), the v1 version of the RCSB search API has been fully discontinued. This PR swaps the library to use the v2 version of the API, following the migration guide detailed here.

The PR also addresses a small number of typos and bugs which are not detailed in the migration guide, but which may be related to the API change.

* "from" -> int
* "to" -> int
* "include_lower" -> bool
* "include_upper" -> bool
Copy link
Author

Choose a reason for hiding this comment

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

It's unclear to me if this is the best change of API that we want in the library, but it is a direct change to the new format specified in v2. Open to suggestions!

@@ -205,7 +207,7 @@ class Terminal(Query):
operator: Optional[str] = None
value: Optional[TValue] = None
service: str = "text"
negation: bool = False
negation: Optional[bool] = False
Copy link
Author

Choose a reason for hiding this comment

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

Ideally I think we should swap this to None as well, but I was afraid of breaking existing workflows which depend on this default. Instead I am passing negation=None in the TextQuery class super().__init__.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I'm now seeing that this might have been a longer-standing error which was raised in another PR:
#5

@sbliven
Copy link
Owner

sbliven commented Jul 25, 2022

@ncoish Thanks for this PR! I'm just coming back from vacation but I'll try to review and merge this soon.

@shervinnia
Copy link

Any update on this? Would be nice to have it merged.

@ncoish
Copy link
Author

ncoish commented Aug 23, 2022

@ncoish Thanks for this PR! I'm just coming back from vacation but I'll try to review and merge this soon.

Hey @sbliven, have you had a chance to take a look at this yet?

@uros-sipetic
Copy link

Heyy will this be merged soon..? The client is super useful, and would be great to have ti working (again)

@joelb123
Copy link

Yes, please update this. All downstream code CI stopped working because of it.

@piehld
Copy link

piehld commented Sep 15, 2023

FYI: #4 (comment)

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.

None yet

6 participants