toml file accepts multiple proxies#156
Conversation
hayleigh-dot-dev
left a comment
hayleigh-dot-dev
left a comment
There was a problem hiding this comment.
Hey thanks for opening this! I've left some comments that need to be resolved, but mostly looks good!
| // CONSTRUCTORS ---------------------------------------------------------------- | ||
|
|
||
| pub fn new(from: String, to: String) -> Result(Proxy, Error) { | ||
| pub fn new(from: String, to: String) -> Result(Proxy, Nil) { |
There was a problem hiding this comment.
I don't think this typechecks anymore, we're annotating that this returns Nil errors but it doesn't!
| proxy_from(flags) |> result.unwrap(""), | ||
| proxy_to(flags) |> result.unwrap(""), | ||
| )) | ||
| let proxies = |
There was a problem hiding this comment.
Because we're throwing away the result here that means we no longer report when a config is invalid which seems like an easy way to confuse folks if they have a typo in their toml!
Reporting all the errors sounds a bit too complicated to tackle here but we should at least report the first problem and bail out here.
There was a problem hiding this comment.
Changed it so that the extraction function now returns an error.Error, and Ok([]) if no proxy is provided rather than an error like it did earlier.
|
Thanks for the feedback! I think the new changes should resolve your comments, but let me know if further tweaks are needed! |
|
Yeah this looks great, thank you! |
2 participants
Adds feature suggested in Issue #148. I tried maintaining the
cliinterface as suggested in the discussion there, but I could not figure out a way to do so without duplicating reads of the config toml.