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

Reordered metadata methods #85

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

charlottekostelic
Copy link
Contributor

Changed default status_forcelist in _session.py
Added more tests to _session.py
Changed method names and reordered MetadataSession

@charlottekostelic charlottekostelic requested a review from klinga March 8, 2024 19:34
Copy link
Member

@klinga klinga left a comment

Choose a reason for hiding this comment

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

That was a lot of work! Impressive! 👏
There are some issues I would like you to look at and consider before merging.

Did not review tests.

bookops_worldcat/metadata_api.py Outdated Show resolved Hide resolved
bookops_worldcat/metadata_api.py Outdated Show resolved Hide resolved
bookops_worldcat/metadata_api.py Outdated Show resolved Hide resolved

return query.response

def bib_get_current_oclc_number(
Copy link
Member

Choose a reason for hiding this comment

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

Worth asking OCLC how many max #s can be sent per one request and provide logic to throw an error when over the cap? vetted_numbers is a list that could easily support it.

bookops_worldcat/metadata_api.py Outdated Show resolved Hide resolved
prepared_request = self.prepare_request(req)

# send request
query = Query(self, prepared_request, timeout=self.timeout)

return query.response

def search_my_holdings(
def local_holdings_search_shared_print(
Copy link
Member

Choose a reason for hiding this comment

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

Wonder what will happen if none of the args are provided as API docs suggest is OK to do. First 10 (?) records are returned but no way to page through subsequent requests? Weird.

Some experimentation would be needed to figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The quality of OCLC's documentation gets worse the further down the page one gets.

bookops_worldcat/metadata_api.py Show resolved Hide resolved
bookops_worldcat/metadata_api.py Outdated Show resolved Hide resolved
bookops_worldcat/metadata_api.py Outdated Show resolved Hide resolved
bookops_worldcat/metadata_api.py Outdated Show resolved Hide resolved
@charlottekostelic
Copy link
Contributor Author

@klinga, I made all of the requested edits and went through the args for each endpoint to clarify which are required and which are optional. In most cases we only have default values for args that OCLC has provided a default value for. The only instances in which this is not the case is with preferredLanguage and inCatalogLanguage which have 'eng' as the default value.

There are some args which are technically optional but one of a few options must be included in a query (eg. summary_holdings_get must contain either oclcNumber, isbn, or issn). I added this and similar details to the doc strings to hopefully make it a bit clearer to users but if you have a suggestion about a better way to document this, let me know. The error messages from the API are mostly helpful but some of them are misleading and suggest using args that aren't actually available for a particular endpoint.

@charlottekostelic charlottekostelic merged commit 05652e1 into releases/v1.0.0 Mar 11, 2024
5 checks passed
@charlottekostelic charlottekostelic deleted the reorder-metadata-methods branch March 11, 2024 20:27
charlottekostelic added a commit that referenced this pull request Mar 26, 2024
* move cov & pytest config to pyproject.toml

* remove temp file

* remove pytest-recording

* add py311

* dependencies updates

* drop py3.7

* copy main config

* drop py3.7

* add requests to dev dependencies

* fix missing dependencies

* dependencies final update

* version bump to 1.0.0

* adds all dependencies

* add py3.12

* typing cleanup & tests refactoring

* Authentication updates (#69)

* ingnore E501 in tests

* add types-requests to dev dependencies

* scopes as str

* scopes as str & types cleanup

* dev dependencies moved to tool.poetry.group.dev.dependencies section

* token_expires_at as datetime obj

* token_expires_at as datetime

* changed utcnow to now(timezone.utc) (#71)

* Ocn-parsing-refactor (#72)

* prep_oclc_numbe_strr refactor

* oclcNumber stripped

* verification refactor

* utc fixes (#73)

* fixed datetime type errors

* moved datetime edits to _hasten_expiration_time

* Errors-refactor (#74)

* removed WorldcatSessionError

* incorrect AttributeError replaced with TypeError

* replaces WorldcatAuthorizationError with TypeError and ValueError for configuration

* removed unused WorldcatAuthorizationError import

* removed unused WorldcatRequestError import

* added safe decoding for bytes-str

* ignore F401

* None type added to possible timeout types

* added type ingnore

* removed unused InvalidOclcNumber import & typing fixes

* changed endpoints in metadata api 2.0 (#77)

* changed endpoints in metadata api 2.0

* fixed tests with typos

* changed response_format default in get_full_bib

* Changed search endpoints (#78)

* changed search endpoints in metadata api 2.0

* fixed types

* fixed spacing and indentation

* type hint fixes and refactored test

* Removed principalID and principalIDNS from token requests (#79)

* removed unnecessary params from token requests

* fixed docstring

* MetadataSession cleanup (#80)

* reordered methods in MetadataSession

* removed obsolete 409 error handling from query.py

* simplified changes

* added new api endpoints (#81)

* added new api endpoints

* added tests

* added test

* added to doc string, fixed typos (#82)

@charlottekostelic This is something that should be brought to users attention in the documentation. Will create an issue as a reminder.
Besides that, looks good. Thanks!

* Query updates (#84)

* work in progress

* added retries to query

* removed test with stale token

* added stale token test back in

* added stale token test back in

* moved retries to _session module, added tests

* added retry status_forcelist tests

* added custom adapter test

* changed default retry behavior

* added another test

* more testing

* Reordered metadata methods (#85)

* renamed/reordered metadata_api methods

* fixed optional/required args, added to doc strings

* fixed default values to match API defaults

* fixed error in live test

* dev status update to 5, removes py3.7 & adds py3.11 & py3.12 (#87)

* Update docs (#88)

* reorganized docs, added mkdocs-material theme

* added css for NYPL colors

* reorganized docs, added examples

* changed structure of docs, added to docs

* changed snake case to camel case in args

* typo fixes

* added contributing.md, python versions for black

* added 1.0 to changelog, migration section in docs

* Added migration section to README

* typo fixes, link fixes

* added py.typed file

* fixed links, made edits per PR 88

* added mkdocstrings, removed mkapi, doc edits (#91)

* edited changelog

* pyproject.toml edits

* update unit-tests.yaml

* fixed unit-tests.yaml

* unit-tests.yaml indentation issues

---------

Co-authored-by: klinga <klingaroo@gmail.com>
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