Skip to content

Use slots instead of directly referencing the attributes #271

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

Merged
merged 6 commits into from
Jun 8, 2021

Conversation

padenot
Copy link
Collaborator

@padenot padenot commented Jun 7, 2021

This does not change any behaviour. I did not do VideoFrame yet because it's moving too much, I'll do it after.

This is split in multiple commit, to make it a lot easier to review.

This fixes almost all of #184, but will need a follow-up for VideoFrame.


Preview | Diff

@padenot padenot requested a review from chcunningham June 7, 2021 13:56
Copy link
Collaborator

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

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

Looks great!

I thought I read somewhere that the style for attribute getters was to say "the getter steps...", but I can't find where I read that and I prefer the style you've used for the simple getters.

VideoFrame should mostly have slots defined already, but maybe I misunderstand what you're planning? You might wait for that one until after the #208 lands... if you're planning lots of churn the merge could get ugly.

@chcunningham chcunningham merged commit 16b948c into main Jun 8, 2021
@chcunningham chcunningham deleted the 184-slots branch June 8, 2021 07:41
github-actions bot added a commit that referenced this pull request Jun 8, 2021
SHA: 16b948c
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jun 8, 2021
SHA: 16b948c
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jun 8, 2021
SHA: 16b948c
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@padenot
Copy link
Collaborator Author

padenot commented Jun 8, 2021

VideoFrame should mostly have slots defined already, but maybe I misunderstand what you're planning? You might wait for that one until after the #208 lands... if you're planning lots of churn the merge could get ugly.

Simply a proofreading to make sure we're good and fix any typos, like the ImageDecoder/complete above, almost nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants