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.

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

OWNERS set for video files in src #25120

Merged
merged 2 commits into from Oct 18, 2019

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Oct 18, 2019

Realized that video service files are really touched when creating 3p player contributions, so they can be set to the broader ampproject/wg-ui-and-a11y group (rather than the specific set from #24908).

Added missing ownership for other video/media related files in src.

@amp-owners-bot
Copy link

Hey @rcebulko, these files were changed:

  • src/OWNERS
  • src/service/OWNERS
  • src/service/video/OWNERS

src/OWNERS Outdated Show resolved Hide resolved
{
rules: [
{
pattern: 'video-*.js',

Choose a reason for hiding this comment

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

Should we move these files into the src/service/video/ folder? The fewer filename patterns the better.

IMO this is a good chance to start massaging our source dir structure into something more sane. Also it would enable semantically grouped subfolder README.md files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some aren't strictly for the service but rather utilities for component implementations.

I agree with the sentiment, though. How about just src/video? Maybe we can move the service implementation into that dir as well.

I'm going to go ahead merging this to unblock another PR but I'll restructure later. Thanks for the suggestions.

Choose a reason for hiding this comment

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

We do have a convention that service classes (objects shared across multiple binaries) are in src/service/ e.g. our linter bans direct imports.

We can change this -- just pointing it out.

@@ -6,5 +6,9 @@
{
owners: [{name: 'dvoytenko'}, {name: 'ampproject/wg-runtime'}],
},
{
pattern: '{iframe-video,mediasession-helper,video-*}.js',

Choose a reason for hiding this comment

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

Ditto.

@alanorozco alanorozco merged commit 2dabd51 into ampproject:master Oct 18, 2019
@alanorozco alanorozco deleted the ownvideoagain branch October 18, 2019 15:44
jeffjose pushed a commit to jeffjose/amphtml that referenced this pull request Oct 19, 2019
Realized that video service files are really touched when creating 3p player contributions, so they can be set to the broader ampproject/wg-ui-and-a11y group (rather than the specific set from ampproject#24908).

Added missing ownership for other video/media related files in src.
joshuarrrr pushed a commit to Parsely/amphtml that referenced this pull request Oct 22, 2019
Realized that video service files are really touched when creating 3p player contributions, so they can be set to the broader ampproject/wg-ui-and-a11y group (rather than the specific set from ampproject#24908).

Added missing ownership for other video/media related files in src.
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
Realized that video service files are really touched when creating 3p player contributions, so they can be set to the broader ampproject/wg-ui-and-a11y group (rather than the specific set from ampproject#24908).

Added missing ownership for other video/media related files in src.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants