-
Notifications
You must be signed in to change notification settings - Fork 27
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
Tests and Updates #41
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
env: | ||
# set environment variables using repo secrets | ||
TRUTHSOCIAL_USERNAME: ${{ secrets.TRUTHSOCIAL_USERNAME }} | ||
TRUTHSOCIAL_PASSWORD: ${{ secrets.TRUTHSOCIAL_PASSWORD }} |
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.
These values need to be set on the upstream repo, using the repository secrets menu in the settings.
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.
Maybe the login isn't really needed: #32
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.
Seems like that's maybe a change from when we first built the tool. Pretty sure it used to all be authwalled.
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.
Passing the username and password allows us to test auth-walled functionality.
We can use a new test account if concerned about security.
@@ -153,9 +182,9 @@ def _get_paginated(self, url: str, params: dict = None, resume: str = None) -> A | |||
|
|||
def user_likes( | |||
self, post: str, include_all: bool = False, top_num: int = 40 | |||
) -> bool | Any: | |||
) -> Union[bool, Any]: |
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.
Fixes error by using Union
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.
hmm, bool | Any is equivalent to Any; also, the return type of this function doesn't make sense to me — shouldn't this return a list of some sort?
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 am not sure about type hints or this function specifically. I had run into an issue with bool | Any
and that was the suggested change. I'm happy to try changing it back.
truthbrush/api.py
Outdated
@@ -209,14 +238,20 @@ def search( | |||
searchtype: str = None, | |||
query: str = None, | |||
limit: int = 40, | |||
resolve: bool = 4, | |||
resolve: bool = 4, # bool = 4 ? |
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.
FYI not sure if bool type and default value of 4 are compatible?
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 was the one who mistakenly introduced this: 1c74a96#diff-b8a4f09258d1b070faf6806097b4e7047930f99777e63d753dc1b03e7eae31a4R117
Needs to be changed ofc.
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.
OK I will change to int
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.
Just curious, what does the resolve do?
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.
No, it needs to be a boolean (defaulted to True
). Not sure what it does, changing from true to false does not change the output at all.
The params I get when executing a search in TS website are:
params = {
'q': 'trump',
'limit': '20',
'resolve': 'true',
'type': 'accounts',
}
So probably resolve needs to be 'true' if resolve else 'false'
in the HTTP req. 1c74a96#diff-b8a4f09258d1b070faf6806097b4e7047930f99777e63d753dc1b03e7eae31a4R124
@@ -248,30 +283,54 @@ def search( | |||
), | |||
) | |||
|
|||
offset += 40 | |||
offset += 40 # use limit 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.
should this be referencing the limit?
FYI this PR is ready for review anytime. Takes some tests and docstring updates from #38 , as requested. |
from curl_cffi import requests | ||
import curl_cffi |
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.
fixes error by importing this package
""" | ||
|
||
params = {} | ||
user_id = self.lookup(username)["id"] | ||
user_id = user_id or self.lookup(username)["id"] |
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.
skips separate request when able
truthbrush/api.py
Outdated
@@ -209,14 +238,20 @@ def search( | |||
searchtype: str = None, | |||
query: str = None, | |||
limit: int = 40, | |||
resolve: bool = 4, | |||
resolve: int = 4, |
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.
updates type hint
|
||
|
||
@pytest.fixture(scope="module") | ||
def user_timeline(client): |
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.
FYI fixtures defined in the conftest.py can get referenced by test functions in other files.
Thanks! Ack, will review shortly. |
Co-authored-by: Konrad Iturbe <[email protected]>
I have started to see some cloudflare restricted pages. I assume when making too many requests in too short a period of time. But does anyone know what the logic is for when they appear? |
Maintenance Updates
Implements Continuous Integration using GitHub Actions, to run tests on CI server. To make this work, you need to set the environment variables as repository secrets (see README.md). See working example here: https://github.com/s2t2/truthbrush/actions
Updates to Python 3.10:
Functionality Updates
Improves documentation and performance of the
pull_statuses
method. Makesuser_name
an optional parameter. Removes unnecessary API call using theusername
- if auser_id
is supplied it uses theuser_id
in the request instead.Improves documentation for the
search
method, designating which resources are valid (closes How to get a group id? #42 ).Adds a simplified search method (
search_simple
) which returns only the resources requested.Adds a datetime conversion utility function which is helpful when asking for posts after a given datetime.