-
Notifications
You must be signed in to change notification settings - Fork 12
CPS-80: Added Directory.list_marketplaces and Directory.get_marketplace #110
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
CPS-80: Added Directory.list_marketplaces and Directory.get_marketplace #110
Conversation
connect/resources/directory.py
Outdated
| :param dict|Query filters: Filters to pass to the request. | ||
| :return: A list with the marketplaces that match the given filters. | ||
| :rtype: list[Asset] |
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.
Missed type here list[Marketplace]
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.
Well spotted. Fixing now.
connect/resources/directory.py
Outdated
| :param str marketplace_id: The id of the marketplace. | ||
| :return: The asset with the given id, or ``None`` if such asset does not exist. | ||
| :rtype: Asset|None |
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.
Missed type, looks like should be Marketplace|None
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.
Fixing...
marcserrat
left a comment
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.
Few changes on descriptions requested
connect/resources/directory.py
Outdated
| """ List the marketplaces. | ||
| :param dict|Query filters: Filters to pass to the request. | ||
| :return: A list with the marketplaces that match the given filters. |
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.
List of marketplaces matching given filters
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.
Fixed.
connect/resources/directory.py
Outdated
| return Marketplace.deserialize(text) | ||
|
|
||
| def get_marketplace(self, marketplace_id): | ||
| """ Returns the marketplace with the given id. |
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.
Obtains Marketplace object given it's ID
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.
Fixed.
connect/resources/directory.py
Outdated
| """ Returns the marketplace with the given id. | ||
| :param str marketplace_id: The id of the marketplace. | ||
| :return: The asset with the given id, or ``None`` if such asset does not exist. |
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.
Asset?
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.
Fixed.
martinconstante
left a comment
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.
It could be great creating a MarkeplaceResource that inherit from BaseResource and includes it in DirectoryResource. (see ProductResource).
Then we will have a resource for each object that matches the API and grouped by facades.
But ProductResource does not contain list or get methods for products itself (only for their parameters). The methods list_products and get_product are still in the Directory class. |
ProductResouce inherits the methods search get create from BaseResource, and implements the specifics methods like the related to parameters. When you want to use the resources of ProductResource you only must include the facade class Directory. The objective is to create a resource class for every business object and group in facade classes like Directory, Fulfillment, Subscription. |
Ok, now I get it. I was getting confused because Directory.get_product and Directory.list_products do not use the Product resource to implement the functionality, but they do it directly with ApiClient. So, should I add a MarketplaceResource class and call the resource methods from Directory.list_marketplaces and Directory.get_marketplace? Correct! |
Thanks, I'll provide the changes ASAP. |
|
Requested changes have been added. |
No description provided.