-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: Update get_articles method to accept integer cursor #27
Conversation
The `get_articles` method in the `OmnivoreQL` class now accepts an integer value for the `cursor` parameter instead of a string. This change improves the clarity and type safety of the method. Additionally, the documentation has been updated to reflect the maximum number of articles that can be fetched and the maximum value for the `limit` parameter.
Reviewer's Guide by SourceryThis pull request updates the File-Level Changes
Tips
|
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.
Hey @yazdipour - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
||
:param limit: The number of articles to return (default is 100). |
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.
suggestion: Clarify the default value for the limit parameter.
The previous docstring mentioned that the default value for the limit parameter is 100. The updated docstring does not specify a default value. If the default is still 100, it would be helpful to retain that information.
:param limit: The number of articles to return (default is 100). | |
:param limit: The number of articles to return (default is 100, max can be 100). |
@@ -104,15 +104,15 @@ | |||
def get_articles( | |||
self, | |||
limit: int = None, | |||
cursor: str = None, |
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.
issue (complexity): Consider maintaining the original type for cursor
and keeping the documentation clear and precise.
The recent changes to the get_articles
function have introduced some complexity and potential issues:
-
Type Change for
cursor
: Changing the type ofcursor
fromstr
toint
can lead to additional type conversions and potential bugs if other parts of the code or external systems expectcursor
to be a string. This also impacts backward compatibility. -
Documentation Clarity: The new docstring is less clear and introduces redundancy. The original docstring provided a detailed explanation of the parameters and was more informative.
To address these issues, consider maintaining the original type for cursor
and keeping the documentation clear and precise. Here is a suggested revision:
def get_subscriptions(self):
"""
Get the subscriptions of the current user.
"""
return self.client.execute(self._get_query("GetSubscriptions"))
def get_articles(
self,
limit: int = None,
cursor: str = None,
format: str = "html",
query: str = "in:inbox",
include_content: bool = False,
):
"""
Get articles for the current user. It is limited to first 100 articles by default. Use limit and cursor to fetch more.
:param limit: The number of articles to return (default is 100).
:param cursor: The cursor to use for pagination.
:param format: The output format of the articles. Can be 'html' (default) or 'markdown'.
:param query: The query to use for filtering articles. Example of query by date: 'in:inbox published:2024-03-01..*'. See https://docs.omnivore.app/using/search.html#filtering-by-save-publish-dates for more information.
:param include_content: Whether to include the content of the articles.
"""
return self.client.execute(
self._get_query("Search"),
variable_values={
"first": limit,
"after": cursor,
"format": format,
"query": query,
"includeContent": include_content,
},
)
This revision maintains the original type for cursor
and keeps the documentation clear and informative.
Summary by Sourcery
This pull request updates the get_articles method to accept an integer cursor for pagination, fixing a bug. Additionally, it clarifies the method's documentation regarding the maximum number of articles that can be fetched and the cursor parameter.