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

[Jiri Vanek]

On 11/29/18 7:14 PM, Alex Kashchenko wrote:
> 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?

Nope. fixed.

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

This is not related to the refacroting.  And the fix is not clear to me. Will elaborate as next
changeset.
> 
> 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).


Rebuke taken.
Will  elaborate here to.

TYVM for review!
  J.
> 
>>
>> [...]
>>
> 
> 
> 


-- 
Jiri Vanek
Senior QE engineer, OpenJDK QE lead, Mgr.
Red Hat Czech
jvanek at redhat.com    M: +420775390109
-------------- next part --------------
A non-text attachment was scrubbed...
Name: refactoredProperties3.patch
Type: text/x-patch
Size: 45321 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20181130/c9d8523e/refactoredProperties3-0001.patch>