-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature: add json output format support #6
base: develop
Are you sure you want to change the base?
Conversation
nipierre
commented
Mar 31, 2021
- Add json output support for more thorough exploitation of data
match input { | ||
"EBU_TT_D" => Ok(OutputFormat::EbuTtD), | ||
"JSON" => Ok(OutputFormat::Json), | ||
_ => Ok(OutputFormat::EbuTtD), |
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.
Maybe you should log a warning message, to notify the specified format is not supported, don't you think?
@@ -89,6 +113,9 @@ impl MessageEvent<WorkerParameters> for TranscriptEvent { | |||
let selected_streams = get_first_audio_stream_id(&format_context)?; | |||
|
|||
let cloned_parameters = parameters; | |||
let param_output_format = cloned_parameters.output_format.clone(); | |||
|
|||
let output_format = OutputFormat::from_str(&(param_output_format.unwrap_or("JSON".to_string()))).unwrap(); |
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.
So the default output format is JSON here but EBU_TT_D at line 81?
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 should also handle the error instead of "unwrap()"
OutputFormat::Json => { | ||
let sequence_index = sequence_number.load(Acquire); | ||
let message = serde_json::to_string(&event).unwrap_or("".to_string()); | ||
info!("{:?}", message); |
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.
The logged message could be more explicit, don't you think? :)
destination_path: String, | ||
source_path: String, | ||
} | ||
|
||
#[derive(Debug, Clone, Deserialize, JsonSchema)] | ||
enum OutputFormat { |
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.
Could you define it into a specific file? To lighten the main file...