Conversation
This requires us providing mapping functions for objects that use SortDirection.
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
LGTM but I have one question where I'm confused about the code diff and commit message.
| function mapCompletedQueryInfoDataToDomainModel( | ||
| completedQuery: CompletedQueryInfoDto, | ||
| ): CompletedQueryInfo { | ||
| const sortState = |
There was a problem hiding this comment.
Not that this mapper looks wrong, but why does renaming the SortDirectionDto type mean we now have to provide a mapper? The type itself hasn't changed but just its name. It doesn't seem like we were accidentally using a DTO type where we shouldn't have been, or accidentally referencing a domain type from the DTO type.
Is this some crazy enum magic, where you can cast them to each other if they are defined in separate files but have the same name?
There was a problem hiding this comment.
Ah yeah sorry I meant to link to microsoft/TypeScript#35138. It seems like before it just happened to work because of the quirk described in that issue.
2 participants
Reviewed names used in the query history store, models (data/domain) and mappers, and updated them to match conventions agreed.
query-historye.g.query-history-domain-mapper.This PR looks fairly big but the changes are pretty small and straight forward. Please review commit by commit to see the differences.
Checklist
N/A:
ready-for-doc-reviewlabel there.