[rfc][icedtea-web] refactored properties loader to be reusable

[Alex Kashchenko]

Hi,

On 11/29/2018 11:35 AM, Jiri Vanek wrote:
> 
> [...]
> 
> Hello!
> 
> All should be fixed, unless it leadked to much form refactroing.

I assume verify_jdk_string should take &str, not &String? Any particular 
reason it uses &String?

In try_key_from_properties_files, can you explain why file is cloned 
multiple times there?

Its input is an a list of strings (you are wrapping them twice, but they 
are still strings underneath), some of which may be empty, and ownership 
of the string is not required to print or format that string. I suggest 
to unwrap the strings (or even better - to not wrap them in the first 
place) before calling the loop logic, because this thing is not nice:

file.clone().unwrap_or(std::path::PathBuf::from("None")).display()

Besides these two points patch looks good, I will review a second patch 
once first one is pushed.


> [...]
>
>   - usage of map. I'm not sure how worthy it is. Mos likly changeset in farer future, mostly for  my
> education (or feel free to add :) )

You are misusing the Option type. It is okay to check it with pattern 
matching when you are getting an Option from API call, when you are 
interested only in actual call result. But if you are using Option 
itself (taking it as a function parameter), the goal of Option type is 
to work on (possibly empty) underlying value without actually checking 
whether the value is there. Option is doing these checks for you 
providing map(), and_then() and other functions.

For example, append_deployment_file() shouldn't take an Option as its 
input, instead it should take a plain value. And then be used with 
Option<...> (though its API) or with plain values where convenient.

It is okay to not use functional-like approach, like chains of map() 
calls (Rust is not a pure-functional lang), but in that case it may be 
better to avoid functional-like APIs like Option (besides the use-case, 
when it is returned and immediately unwrapped).

>
> [...]
>



-- 
-Alex