-
Couldn't load subscription status.
- Fork 236
Add recommendations backend boilerplate #4301
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 recommendations backend boilerplate #4301
Conversation
| class Recommendations(Backend): | ||
|
|
||
| def connect(self) -> None: | ||
| return super().connect() |
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 just curious, assuming that we would be needing this connect method we might want to check in other functions that request, and make_request if the connection is currently alive? if its not then we may want to establish the connection first? I believe this should not be needed in the class code itself and should be handled by the developer while using the class but a safety check before making requests can be useful?
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.
Yes @ozer550 you raise a great point. I am fine with either removing it or it remaining. Though the latter enforces what needs to be done and rids the make_request from any connection detail(Single responsibility pattern!!). I am almost certain all backends have some form of connection. Its very unlikely that they keep their door open to you all the time 🙂
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.
yes, this makes sense! I think we should let it remain 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.
Added some comments
| class Recommendations(Backend): | ||
|
|
||
| def connect(self) -> None: | ||
| return super().connect() |
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.
Yes @ozer550 you raise a great point. I am fine with either removing it or it remaining. Though the latter enforces what needs to be done and rids the make_request from any connection detail(Single responsibility pattern!!). I am almost certain all backends have some form of connection. Its very unlikely that they keep their door open to you all the time 🙂
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!
Summary
Description of the change(s) you made
This PR adds
Recommendationsbackend with its boilerplate.Manual verification steps performed
Not applicable under the circumstances
Does this introduce any tech-debt items?
Reviewer guidance
How can a reviewer test these changes?
References
Closes #4296
Comments
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)