-
Notifications
You must be signed in to change notification settings - Fork 46
migrate new GitHub users API #290
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
Conversation
ZibanPirate
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.
this is a well written PR, couple more changes suggested, and we good to go 👌🏽
api/src/github-user/controller.ts
Outdated
| const githubUser = await this.githubService.getUser({ | ||
| username, | ||
| }); | ||
| const user = plainToClass(GithubUserDto, { |
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 is no need to verify a service response, so the code from line 25 to 27 can be removed as well as on line 7
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.
yesss! I've just fixed it.
but can I ask a question? I added these lines for documentation. if I do not specify class object like a GithubUserDto into ResponseDto, I couldn't see any details in http://localhost:7070/v2/docs/ are there any issues when a user object is wrapped in a DTO object? It should be tiresome, but I would be learned lots of things from your reply :)
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, as i mentioned in the new comments, the nested objects inside the dto class has to be dto classes too, otherwise OpenApi will not pick them as specs in the docs, you also need to decorate them with @ValidateNested() (or @ValidateNested({ each: true }) and @Type(() => YouClassDto) for array of objects)
Co-authored-by: Zakaria Mansouri <zakman.dev@gmail.com>
ZibanPirate
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.
one last change and we're good to go!
Co-authored-by: Zakaria Mansouri <zakman.dev@gmail.com>
Co-authored-by: Zakaria Mansouri <zakman.dev@gmail.com>
Co-authored-by: Zakaria Mansouri <zakman.dev@gmail.com>
Co-authored-by: Zakaria Mansouri <zakman.dev@gmail.com>
Type 'GitHubUserApiResponse' is not assignable to type 'GithubUserDto'.
Types of property 'id' are incompatible.
Type 'number' is not assignable to type 'string'.I think It's a conflict between export class GithubUserDto implements GithubUser {
...
@IsString()
id!: string;
...
}
export interface GitHubUserApiResponse {
...
id: number;
...
} |
ZibanPirate
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.
yes you're correct, we need to do 3 changes:
- in
common/src/types/index.tschange line 43 to:
id: number;- in
api/src/github/dto.tschange line 10 to:
id!: number;- in
api/test/mocks.tschange lines 6, 14 and 22 to:
id: 1, id: 2, id: 3,|
@ZibanPirate I'm sorry to late 😭 I've just implemented earlier suggestion :) |
ZibanPirate
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.
no worries, thanks for your well balanced PR ;) merging ...
migrate new GitHub users API
Description
Implements #286
v2/GithubUsers/{username}from the frontend/github/usersType of change