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

Align get_indexes() with API spec #114

Draft
wants to merge 1 commit into
base: mainline
Choose a base branch
from

Conversation

OwenPendrighElliott
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Bug fix

  • What is the current behavior? (You can also link to an open issue here)
    Currently get_indexes returns a dictionary of str and list of Index objects. The docstring and the REST API however return a dictionary of str and a list of dicts of str and str.

e.g. the python client returns

{'results': [<marqo.index.Index object at 0x103f65090>]}

Where the REST API returns

{'results': [{"index-name", "my-first-index"}]}
  • What is the new behavior (if this is a feature change)?
    The new behaviour is to have get_indexes() return the same structure as the REST API
>>> mq.get_indexes()
{'results': [{"index-name", "my-first-index"}]}
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    This PR does break the use case where users use the Index objects returned from get_indexes().

e.g.

>>> indexes = mq.get_indexes()['results']
>>> for index in indexes:
...    print(index.get_stats())
>>>

However this usage pattern is never documented anywhere or used in any examples as far as I can see. It only appears in one of the tests for the get_indexes() method.

  • Other information:
    This changes means that you can print the results as dictionaries have __repr__.

An alternative would be to add __getitem__ and __repr__ to the index object to retain both the existing functionality and let users treat it like a dict. e.g.

>>> indexes = mq.get_indexes()['results']
>>> for index in indexes:
...    print(index.get_stats())
>>> print(indexes)
[Index(config=<marqo.config.Config object at 0x10382f610>, http=<marqo._httprequests.HttpRequests object at 0x102682b10>, index_name='my-first-index', created_at=None, updated_at=None)]
>>> indexes[0]['index_name']
'my-first-index'

@OwenPendrighElliott
Copy link
Contributor Author

@pandu-k what are your thoughts on this? I think we discussed it about 2 months ago but I never got around to taking in further.

@OwenPendrighElliott OwenPendrighElliott changed the title changed return type for get_indexes to match the rest API Align get_indexes() with API spec Jul 6, 2023
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.

1 participant