-
Notifications
You must be signed in to change notification settings - Fork 18
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
Implemented autocomplete endpoint and added documentation #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR and the documentation! It is pleasure to have a feature-complete library soon :)
I never took the effort to implement it, as, in my opinion, the design of the autocomplete API of OpenAlex is not very intuitive (autocomplete looks like a resource instead of a method). I'm happy with your abstraction autocomplete. This makes things more straightforward. I have a couple of remarks on your implementation.
It might also be good to add a unit test. Something like this:
def test_autocomplete():
w = Works().autocomplete("open ale")
assert not isinstance(w[0], Work)
README.md
Outdated
from pyalex import Autocompletes | ||
|
||
Autocompletes()["stockholm"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from pyalex import Autocompletes | |
Autocompletes()["stockholm"] | |
from pyalex import autocomplete | |
autocomplete("stockholm") |
My suggestion would be to rewrite it into this as autocomplete is not a separate resource (although the OpenAlex API design might let users think this).
With this change, we reserve the item retrieval for identifiers.
pyalex/api.py
Outdated
def _full_collection_name(self, autocomplete=False): | ||
if autocomplete: | ||
base_url = config.openalex_url + "/autocomplete/" | ||
return base_url + self.__class__.__name__.lower() | ||
else: | ||
return config.openalex_url + "/" + self.__class__.__name__.lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify this and keep it unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could replace if autocomplete:
by if 'q' in self.params.keys():
pyalex/api.py
Outdated
if 'q' in self.params.keys(): | ||
# if q is in params, then it means that an autocomplete query was asked | ||
autocomplete = True | ||
else: | ||
autocomplete = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And delete this part
def autocomplete(self, s, **kwargs): | ||
""" autocomplete the string s, for a specific type of entity """ | ||
self._add_params("q", s) | ||
return self.get(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion would be to implement it like this:
def autocomplete(self, s, **kwargs): | |
""" autocomplete the string s, for a specific type of entity """ | |
self._add_params("q", s) | |
return self.get(**kwargs) | |
def autocomplete(self, q, return_meta=False): | |
"""Autocomplete query q for entity""" | |
self._add_params("q", q) | |
# manipulate URL for autocomplete | |
url_split = urlsplit(self.url) | |
new_url = urlunsplit( | |
( | |
url_split.scheme, | |
url_split.netloc, | |
f"/autocomplete{url_split.path}", | |
url_split.query, | |
url_split.fragment, | |
) | |
) | |
return self._get_from_url( | |
new_url, return_meta=return_meta, resource_class=Autocomplete | |
) |
Also, it's not the nicest option, but it keeps the autocomplete functionality separated from everything else. We don't need the changes above. As I don't think this was the most intuitive implementation OpenAlex chose, we might benefit from keeping it separate from the rest of the class.
I changed _get_from_url
to accept an extra argument.
pyalex/api.py
Outdated
class Autocompletes(BaseOpenAlex): | ||
""" Class to autocomplete without being based on the type of entity """ | ||
resource_class = Autocomplete | ||
|
||
def __getitem__(self, key): | ||
return self._get_from_url( | ||
config.openalex_url + "/autocomplete" + "?q=" + key, return_meta=False | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we can make this into something like this:
def autocomplete(q, return_meta=False):
BaseOpenAlex().autocomplete(q, return_meta=return_meta)
I expect this to require some changes to the base class as well.
pyalex/api.py
Outdated
@@ -212,9 +222,10 @@ def url(self): | |||
l_params.append(k + "=" + quote_plus(str(v))) | |||
|
|||
if l_params: | |||
return self._full_collection_name() + "?" + "&".join(l_params) | |||
return self._full_collection_name( | |||
autocomplete=autocomplete) + "?" + "&".join(l_params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove autocomplete=autocomplete
pyalex/api.py
Outdated
|
||
return self._full_collection_name() | ||
return self._full_collection_name(autocomplete=autocomplete) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
The first reason why I implemented the autocomplete as the other entities was to benefit from the already existing functions of the library like paging. I just did some tests and realized it's not implemented on the OpenAlex API. The only parameters for autocomplete are The other reason was to reuse as much code as possible but as you shown this may not be the only solution. I'm still not sure whether keeping my changes in From my perspective, managing the autocomplete in |
@J535D165 I implemented the changes and added 2 tests. The changes made passed the tests with python 3.10 on my computer (GitHub seems to have trouble running the tests, as the error 429 too many requests occur) |
Thanks @romain894. Planning on a new release soon! |
The library now fully supports the autocomplete endpoint. I added a section in the README with the documentation on how to use it.
I also added a section in the README to explain how to set up the config to enable the retry.