-
Notifications
You must be signed in to change notification settings - Fork 230
Implement Recommendations Adapter #4303
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
Implement Recommendations Adapter #4303
Conversation
cc @akash5100 |
pass | ||
|
||
|
||
class RecommendationsBackendFatory(BackendFactory): |
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.
class RecommendationsBackendFatory(BackendFactory): | |
class RecommendationsBackendFactory(BackendFactory): |
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 spot @akash5100. I will make the changes to this
@abstractmethod | ||
def make_request(self, url: str, params=None) -> Union[bytes, str, Dict]: | ||
""" Makes an HTTP request to a given URL using the specified method. """ | ||
def make_request(self, request) -> Union[bytes, str, Dict]: |
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.
def make_request(self, request) -> Union[bytes, str, Dict]: | |
def make_request(self, request: requests.Request) -> requests.Response: |
If we are going to use requests.Request
object to make request, in the base class should we add this?
and here:
def request(self) -> requests.Request:
as this method is used to create requests.Request
object. and
def response(self) -> Union[bytes, str, Dict]:
this will return response (Json, Str, Binary)
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.
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.
Hi @akash5100! Please refer to our discussion on slack. The object we are referring to here is the POJO equivalent in Java but this time in python. Also, we can alter the return type of the abstract method in our implementations to return whatever we want.
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.
We'll clean this up so we can return either an object
or array
+ any other that we anticipate being returned by the backends
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.
Okay, thanks
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.
Alternatively, we can create base backend objects(Which i think is better)from which we can create the other specific backend objects.
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 the Recommendations adapter. It also moves and deletes some redundant files.
Manual verification steps performed
NA
References
Closes #4302
Comments
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
)