-
Notifications
You must be signed in to change notification settings - Fork 154
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
Core functionality of connect
and make_request
methods on Backend
class
#4494
Core functionality of connect
and make_request
methods on Backend
class
#4494
Conversation
6ca86a0
to
283abc6
Compare
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.
For the most part, it looks great! Mostly some small things. The most important things that could use attention are:
- the issue with
url_prefix="", *args, **kwargs
- exception logging
- retry behavior
- repeat calls to
__init__
I have merged #4499 so rebasing this PR on the current search-recommendations branch should fix the build issue that was observed here! |
b7c7a3f
to
267935c
Compare
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.
Looking great! Sorry for the late followup. I responded to your question about the timeouts.
Thank you @bjester!! Just added the timeout to the backend request 👐. |
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.
Revisiting this I caught more things. Sorry for the back and forth. I think this will be the last before approval.
Thank you @bjester! I have addressed the comments 👐. |
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.
Nice work!
c77b320
into
learningequality:search-recommendations
Summary
Description of the change(s) you made
Implements the core funcitonlity and error handling of the
connect
andmake_request
methods on theBackend
class.Manual verification steps performed
Created various sub-classes of the
Backend
class testing itsconnect
andmake_requests
methods.Reviewer guidance
How can a reviewer test these changes?
Play with the
Backend
class, making sub-classes in thetest_base.py
fileReferences
Closes #4447
Contributor's Checklist
PR process:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)