-
Notifications
You must be signed in to change notification settings - Fork 111
add support for -containsSyntheticMedia #210
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
cmd/youtubeuploader/main.go
Outdated
| notifySubscribers := flag.Bool("notify", true, "notify channel subscribers of new video. Specify '-notify:=false' to disable.") | ||
| debug := flag.Bool("debug", false, "turn on verbose log output") | ||
| sendFileName := flag.Bool("sendFilename", true, "send original file name to YouTube") | ||
| containsSyntheticMedia := flag.Bool("containsSyntheticMedia", false, "video contains synthetic media") |
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.
I would prefer not to add any more flags that relate to video metadata, and instead leave those kinds of things in the metadata JSON file only. At some point in the future I may overhaul the cli and include every option as a flag. For now, please remove this flag.
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.
Removed :-)
Please check.
cmd/youtubeuploader/main.go
Outdated
| SendFileName: *sendFileName, | ||
| PlaylistIDs: playlistIDs, | ||
| RecordingDate: recordingDate, | ||
| ContainsSyntheticMedia: *containsSyntheticMedia, |
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.
Please remove (see earlier comment about flags).
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.
Removed :-)
Please check.
test/upload_test.go
Outdated
| func TestContainsSyntheticMedia(t *testing.T) { | ||
| url, err := url.Parse(testServer.URL) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
|
|
||
| transport := &mockTransport{url: url} | ||
| limitTransport, err := limiter.NewLimitTransport(transport, limiter.LimitRange{}, fileSize, 0) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| config.ContainsSyntheticMedia = true | ||
| ctx := context.Background() | ||
| videoReader := &mockReader{fileSize: fileSize} | ||
| defer videoReader.Close() | ||
|
|
||
| err = yt.Run(ctx, limitTransport, config, videoReader) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| } | ||
|
|
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.
I appreciate you making the effort to create a test, however this test doesn't really achieve anything useful. Please remove it. (I don't think it's really necessary to have tests for all the video properties)
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.
You're totally right, sorry about this brain cramp, removed :)
files.go
Outdated
| NotifySubscribers bool | ||
| SendFileName bool | ||
| RecordingDate Date | ||
| ContainsSyntheticMedia bool |
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.
Please remove (see earlier comment about flags).
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.
Removed :)
Please check.
http.go
Outdated
| PublicStatsViewable bool `json:"publicStatsViewable,omitempty"` | ||
| PublishAt Date `json:"publishAt,omitempty"` | ||
| MadeForKids bool `json:"madeForKids,omitempty"` | ||
| ContainsSyntheticMedia *bool `json:"containsSyntheticMedia,omitempty"` |
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.
Please change this to not be a pointer.
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.
Changed. Please check :)
files.go
Outdated
| if config.ContainsSyntheticMedia { | ||
| video.Status.ContainsSyntheticMedia = true | ||
| } | ||
|
|
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.
Please remove (see earlier comment about flags).
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.
Done, please check :)
files.go
Outdated
| if videoMeta.PublicStatsViewable { | ||
| video.Status.PublicStatsViewable = videoMeta.PublicStatsViewable | ||
| } | ||
| if videoMeta.ContainsSyntheticMedia != nil { |
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 shouldn't be a pointer. Instead use if videoMeta.ContainsSyntheticMedia {. Also, please update the example metadata JSON in the README to include this option.
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.
Changed to match MadeForKids logic. Also added example to README.md. Please check :)
porjo
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.
lgtm, thanks!
Add support for -containsSyntheticMedia via CLI or via JSON (with -metaJSON flag).
In the backend will add the flag to status.containsSyntheticMedia.