-
Notifications
You must be signed in to change notification settings - Fork 834
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
add trailing slash to the end of the url #1594
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 for taking the time to make this PR! Can you apply the same change to legacy_base_client.py for consistency?
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.
LGTM; I will be offline for a while but other maintainers will merge this once the CI builds pass. Thank you for taking the time to fix this issue!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1594 +/- ##
==========================================
- Coverage 84.91% 84.89% -0.03%
==========================================
Files 113 113
Lines 12622 12628 +6
==========================================
+ Hits 10718 10720 +2
- Misses 1904 1908 +4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
lgtm, thank you 💯
Thank you for all the support 🙏 |
Hi @WilliamBergamin and @seratch I noticed that after I changed the I just submitted the issue with the PR #1596 in order to fix it. Apologies for the mistake. |
No worries 🤔 I had a feeling these changes may affect more edge cases then we could think of My concern is that our unit tests did not catch this edge case and may have missed others that we are unaware of, It may be simpler to revert this commit, add more tests around this area in a separate PR and reopen these changes with updates to the tests |
This reverts commit c82c9a0.
Summary
This PR addresses issue #1541 by ensuring that a trailing
/
is appended to the end of the URL in base_url if it is not already provided. This change ensures consistent URL formatting and avoids potential issues caused by missing trailing slashes in API calls.Testing
Run the following command to ensure all relevant test cases pass:
base_url
without a trailing/
in a sample implementation. Confirm that the URL is automatically appended with a/
during API calls.base_url
with a trailing/
and verify that no additional/
is appended.Category
/docs
(Documents)/tutorial
(PythOnBoardingBot tutorial)tests
/integration_tests
(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh
after making the changes.