Allow parameters in any order

[talbotmcinnis]

Used a new style of regex parsing to be more flexible about how options are parsed for each object type. This sets the ground work for more flexibility to define new options going forward.

[rkusa]

This is going to be a very useful improvement to spot configuration issues, thanks a lot for the PR! 👍

Looks great, I only left some minor comments for the first method, but they would also apply to the other ones. Let me know what you think about the suggested changes.

[talbotmcinnis]

Good stuff. I'll read through the feedback and integrate as much as I can. Cheers.

…

On Thu, Dec 3, 2020 at 9:16 AM Markus Ast ***@***.***> wrote: ***@***.**** commented on this pull request. This is going to be a very useful improvement to spot configuration issues, thanks a lot for the PR! 👍 Looks great, I only left some minor comments for the first method, but they would also apply to the other ones. Let me know what you think about the suggested changes. ------------------------------ In crates/datis-core/src/extract.rs <#73 (comment)>: > - .and_then(|s| TextToSpeechProvider::from_str(s.as_str()).ok()); - let info_ltr = caps - .get(10) - .map(|ilo| ((ilo.as_str()).chars().next().unwrap().to_ascii_uppercase()) as char); - StationConfig { - name: name.to_string(), - atis: atis_freq, - traffic: traffic_freq, - tts, - info_ltr_override: info_ltr, + let re = RegexBuilder::new(r"^ATIS ([a-zA-Z- ]+) ([1-3]\d{2}(\.\d{1,3})?)") + .case_insensitive(true) + .build() + .unwrap(); + + let caps = re.captures(config).unwrap(); Since we cannot know if this will succeed, we have to return None instead of panicking here ⬇️ Suggested change - let caps = re.captures(config).unwrap(); + let caps = re.captures(config)?; (the other unwraps in this method should be fine since we know that they will always succeed, but it would actually also not hurt to use ? for them) ------------------------------ In crates/datis-core/src/extract.rs <#73 (comment)>: > + let caps = rex_option.captures(token.trim()).unwrap(); + let option_key = caps.get(1).unwrap().as_str(); + let option_value = caps.get(2).map_or("", |m| m.as_str()); Might not be necessary to use regex here ⬇️ Suggested change - let caps = rex_option.captures(token.trim()).unwrap(); - let option_key = caps.get(1).unwrap().as_str(); - let option_value = caps.get(2).map_or("", |m| m.as_str()); + let token = token.tim(); + let (key, value) = token.split_at(token.find(',').unwrap_or(token.len())); (code is untested) ------------------------------ In crates/datis-core/src/extract.rs <#73 (comment)>: > + let mut traffic_freq: Option<u64> = None; + let mut tts: Option<TextToSpeechProvider> = None; + let mut info_ltr_override = None; + + let rex_option = RegexBuilder::new(r"([^ ]*) (.*)") + .case_insensitive(true) + .build() + .unwrap(); + for token in config.split(",").skip(1) { + let caps = rex_option.captures(token.trim()).unwrap(); + let option_key = caps.get(1).unwrap().as_str(); + let option_value = caps.get(2).map_or("", |m| m.as_str()); + + match option_key { + "TRAFFIC" => { + if let Some(traffic_freq_hz) = option_value.parse::<f64>().ok() { ⬇️ Suggested change - if let Some(traffic_freq_hz) = option_value.parse::<f64>().ok() { + if let Ok(traffic_freq_hz) = option_value.parse::<f64>() { ------------------------------ In crates/datis-core/src/extract.rs <#73 (comment)>: > + let option_key = caps.get(1).unwrap().as_str(); + let option_value = caps.get(2).map_or("", |m| m.as_str()); + + match option_key { + "TRAFFIC" => { + if let Some(traffic_freq_hz) = option_value.parse::<f64>().ok() { + traffic_freq = Some((traffic_freq_hz * 1_000_000.0) as u64); + } else { + log::warn!( + "Unable to extract ATIS station traffic frequency from {}", + option_value + ); + } + } + "VOICE" => { + if let Some(tts_provider) = TextToSpeechProvider::from_str(option_value).ok() { ⬇️ Suggested change - if let Some(tts_provider) = TextToSpeechProvider::from_str(option_value).ok() { + if let Ok(tts_provider) = TextToSpeechProvider::from_str(option_value) { — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#73 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYYIGRYYZTIYYQSEJ7CVGTSS6MULANCNFSM4T6HL7PA> .

[rkusa]

Sorry that it took so long, looks good, thanks!