-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Refactor Catalog API #659
base: main
Are you sure you want to change the base?
Refactor Catalog API #659
Conversation
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.
Left some comments inline!
Do we want to create a new API version for the catalog API and make these changes in a new version of this API? It would be a good way to dogfood the versioning experience.
api.MapPost("/items", CreateItem) | ||
.WithName("CreateItem") | ||
.WithSummary("Create a catalog item") | ||
.WithDescription("Create a new item in the catalog"); | ||
api.MapPut("/items/{id:int}", UpdateItem) |
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.
Do we want to retain the Items
tag on this and the other items-related endpoints?
{ | ||
var pageSize = paginationRequest.PageSize; | ||
var pageIndex = paginationRequest.PageIndex; | ||
|
||
var totalItems = await services.Context.CatalogItems | ||
var root = (IQueryable<CatalogItem>)services.Context.CatalogItems; |
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.
Do we need the cast to IQueryable<CatalogItem>
here?
.LongCountAsync(); | ||
|
||
var itemsOnPage = await services.Context.CatalogItems | ||
.OrderBy(c => c.Name) | ||
var itemsOnPage = await root |
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.
Looks like the ordering-by-name as removed here. Do we want to keep that?
@@ -64,64 +64,10 @@ | |||
} | |||
] | |||
}, | |||
"route4": { |
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.
Should we keep the api-version query parameter here?
This PR refactors the Catalog API to adopt more commonly accepted REST API conventions.
Fixes #589.