-
Notifications
You must be signed in to change notification settings - Fork 51
Added ItemSearch.url_with_parameters #304
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
Conversation
Codecov ReportBase: 84.90% // Head: 84.98% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #304 +/- ##
==========================================
+ Coverage 84.90% 84.98% +0.08%
==========================================
Files 11 11
Lines 775 786 +11
==========================================
+ Hits 658 668 +10
- Misses 117 118 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
@wietzesuijker I've made a couple of changes to your branch:
- Fixed some typing issues
- I use requests to build the URL instead of doing it ourselves
I've force-pushed these changes, so you'll have to do a hard reset of your branch if you want to make any additional changes.
@scottyhq would you mind looking this over to make sure this is what you want?
I'm going to add an additional reviewer since I made some non-insignificant changes to the PR.
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, but it'd be great to include a docstring. I believe it'll automatically show up in the api.rst
.
Related Issue(s):
Description:
PR Checklist: