catalog.roblox.com endpoint support
#111
Conversation
We have been using 3.8-exclusive stuff for a bit now.
tomodachi94
force-pushed
the
catalog-api
branch
from
e83626c to
e93aa7c
Compare
tomodachi94
force-pushed
the
catalog-api
branch
from
e93aa7c to
d814ef7
Compare
|
|
||
| self._client: Client = client | ||
| self.id: int = catalog_item_id | ||
| self.item_type: int = catalog_item_type |
There was a problem hiding this comment.
Where is this coming from?
There was a problem hiding this comment.
I was in the process of converting the itemType stuff to a class, I committed this on accident before the rest was ready
| from ..client import Client | ||
|
|
||
|
|
||
| class BaseCatalogItem(BaseItem): |
There was a problem hiding this comment.
catalog items are either bundles or assets. This approach of one unified class makes it impossible to invoke methods that BaseAsset or a hypothetical BaseBundle would support even when applicable. Maybe forego this class entirely, introduce a BaseBundle (which we need anyway), and make "catalog items" just a BaseAsset | BaseBundle union? Or, instead of a union, both could derive from a new BaseCatalogItem class with a @property that indicates what "type" of catalog item it is (asset or bundle) determined by isinstance(self, BaseAsset): "asset" etc. not sure.
| Attributes: | ||
| id: The item ID. | ||
| item_type: The item's type, either 1 or 2. |
There was a problem hiding this comment.
Make an enum for this: Asset or Bundle.
| catalog_item_data = catalog_item_response.json() | ||
| catalog_list: Literal[CatalogItem] = [] | ||
| for catalog_item in catalog_item_data: | ||
| if data["collectibleItemId"]: # This is the only consistent indicator of an item's limited status |
There was a problem hiding this comment.
There are still "classic Limiteds" that are not collectibles, which makes this inaccurate. Either way, I think just ditch the two classes and go with one CatalogItem class for both. Makes more sense to me even if the Python type system might not be able to imply that multiple properties must exist if one does. If we end up keeping this, call it a CollectibleCatalogItem instead for accuracy.
|
|
||
| return catalog_list | ||
|
|
||
| def get_base_catalog_items(self, catalog_item_array: List[TypedDict[catalog_id: int, catalog_item_type: Literal[1, 2]]]) -> List[CatalogItem]: |
There was a problem hiding this comment.
ditch the typeddict with the first comment's solutions, ditch the 1,2 for the enum
| catalog_list: Literal[CatalogItem] = [] | ||
|
|
||
| for catalog_item in catalog_item_array: | ||
| catalog_list.append(BaseCatalogItem(client=self, data=catalog_item)) |
There was a problem hiding this comment.
for code quality and appearance, use a list comprehension here: return [BaseCatalogItem(client=self, data=catalog_item) for catalog_item in catalog_item_array]
| self.previous_usernames: List[str] = data["previousUsernames"] | ||
|
|
||
|
|
||
| class CatalogCreatorPartialUser(PartialUser): |
There was a problem hiding this comment.
I don't think a new class is warranted here. I think, unless it has extra fields, all PartialUser-shaped objects should just be PartialUsers. Maybe PartialUser could optionally take fields directly , like PartialUser(client=..., id=..., name=..., has_verified_badge=...) so we don't have to make more classes.
I fixed most of the things that won't require huge fixes. I still have a lot of work to do on this PR, but this is a start.
2 participants
This adds support for the
catalog.roblox.comendpoint. Related changes include:CatalogItemandLimitedCatalogItem.CatalogItem, so I figured a separate class would aid in maintainability.partials.partialuserandpartials.partialgroupto include two new classes that handle user/group data from this endpoint.exceptions.CatalogItemNotFound.client.Client,get_catalog_itemsandget_base_catalog_items.get_catalog_itemandget_base_catalog_item.Things left to do
CatalogItemandLimitedCatalogItemcatalog.CatalogItemTypescategoriessubcategoriesasset-to-categoryget-topics