Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fear(parser): Add DecoratedAvatarView #544

Merged
merged 7 commits into from
Dec 27, 2023
Merged

fear(parser): Add DecoratedAvatarView #544

merged 7 commits into from
Dec 27, 2023

Conversation

iBicha
Copy link
Contributor

@iBicha iBicha commented Nov 27, 2023 •
edited
Loading

Added DecoratedAvatarView as I was getting a warning.

Then I got a parsing error

ParsingError: Type mismatch, got DecoratedAvatarView expected ContentPreviewImageView.

Updating PageHeaderView seems to fix the error

@absidue
Copy link
Collaborator

absidue commented Nov 27, 2023

I'm guessing this was copied straight from the the generator?

The generator doesn't currently handle view models properly, which is why it didn't tell you that you should have a separate class for AvatarView. Also instead of manually parsing the array of images, please do it the same way that is done here: https://github.com/LuanRT/YouTube.js/blob/main/src/parser/classes/ContentPreviewImageView.ts#L13

@iBicha
Copy link
Contributor Author

iBicha commented Nov 27, 2023

I'm guessing this was copied straight from the the generator?

Yes, as I'm fairly new to this codebase.
Thanks for the feedback

@iBicha iBicha marked this pull request as ready for review December 8, 2023 23:17
@iBicha iBicha requested a review from LuanRT December 8, 2023 23:17
absidue
absidue reviewed Dec 22, 2023
View reviewed changes
Copy link
Collaborator

@absidue absidue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small nitpicks so simplify the structure, other than that it looks good to me.

src/parser/classes/DecoratedAvatarView.ts Outdated Show resolved Hide resolved
src/parser/classes/DecoratedAvatarView.ts Outdated Show resolved Hide resolved
src/parser/classes/DecoratedAvatarView.ts Outdated Show resolved Hide resolved
src/parser/classes/DecoratedAvatarView.ts Outdated Show resolved Hide resolved
src/parser/classes/DecoratedAvatarView.ts Outdated Show resolved Hide resolved
@iBicha @absidue
Apply suggestions from code review
1aa827c 
Co-authored-by: absidue <48293849+absidue@users.noreply.github.com>
@iBicha
Copy link
Contributor Author

iBicha commented Dec 23, 2023

Just a few small nitpicks so simplify the structure, other than that it looks good to me.

Thanks, I wanted to simplify but I didn't know how much the wrapper code needed to be faithful to the original model.
These changes definitely makes things better

@LuanRT LuanRT changed the title Add DecoratedAvatarView fear(parser): Add DecoratedAvatarView Dec 27, 2023
@LuanRT LuanRT merged commit 9618f38 into LuanRT:main Dec 27, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absidue absidue absidue left review comments

@LuanRT LuanRT LuanRT approved these changes

Assignees
No one assigned
Labels
parser small-diff
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@iBicha @absidue @LuanRT