-
Notifications
You must be signed in to change notification settings - Fork 111
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
Feat: Add pagination to position history endpoint #401
Conversation
Ahh the good old failing tests. Will have to refactor them for the modified changes. I will take some time for those. |
Some guidance on how I should approach for the tests to pass will be appreciated. I am still a bit ambiguous on how I should tackle the test cases. I can try to modify the functions so they integrate well the existing test cases, or refactor the test cases to accommodate the functional changes? Please tell me which route would be a better option. |
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.
Regarding test cases, you can adjust them based on your changes
web_app/api/position.py
Outdated
|
||
positions = position_db_connector.get_positions_by_wallet_id(wallet_id) | ||
|
||
if start is not 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.
if start is not None:
start_index = max(0, start)
else:
start_index = 0
this all code can be done in one line:
start_index = start and max(0, start) or 0
@KeneePatel Any updates, |
@KeneePatel Any updates? Just friendly reminder that if you don't update PR you can be unassigned |
It was busy today. I will update the PR in 6 hours. |
@KeneePatel okay, thank you for update |
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.
Looks good to me
Changes include:
get_user_positions
function endpoint now has an optionalstart
argument for accepting query parameter ofstart
get_positions_by_wallet_id
db function now acceptsstart
, andlimit
parameters to accommodate the changeNo breaking changes
Modification to a functionality is done and was to be expected due to the nature of issue.