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

[Alex Kashchenko]

Hi,

On 11/20/2018 02:54 PM, Jiri Vanek wrote:
> this refactoring is doing two things (Sorry)
>   - first and msot important, is refactoring jre from properties methods to be generic properties
> loader (and validator)

I suggest to drop Validator and write concrete validation functions 
instead (only a suggestion, feel free to keep it).


>   - during doing so, I had realised that  system config files are heavilky os depndent, and so I
> moved the methods to trait OS and implemented for linux. I'm afraid it is msut to do anyway.


1. dirs_paths_helper

  - I think it is better to not use env::home_dir(), it is quite a weird 
function (goes first to env vars, next to OS API). I suggest using 
either env vars (HOME and USERPROFILE), or OS API for that to get more 
predictable results.

  - in get_xdg_config_dir please use "_" for unused matches (that is a 
common convention)

  - please change append_deployment_file function, there is an 
Option#map method to operate on underlying value of Option. If such 
functional-like usage is undesired - I suggest changing the signature to 
PathBuf -> PathBuf (also String -> String may be even cleaner, but that 
is a matter of taste).

  - indentation on "let os = os_access::Linux::new(false);" is 
inconsistent (probably forgot to apply :retab )


2. property_from_file

  - in get_fail_message please move string literals into format strings, 
number of format string also can be lesser (one single format string is 
probably inconvenient here)

  - please drop is_file, I suggest using 
path.metadata().map(|md|md.is_file()).unwrap_or(false) instead

  - get_property_from_file: either use Option#map or expose 
get_property_from_file_direct instead

  - get_property_from_file_direct: if error message is not used in 
File#open, then it is better to use Result#ok and then map the obtained 
Option as required

  - please drop verify_jdk_string, instead pass string ref to 
verify_jdk_path


  - inconsistent validation in impl Validator line and in 
tests::get_jre_from_file


3. property_from_files_resolver

  - try_key_from_properties_files: please move string literals into 
format strings

  - inside try_key_from_properties_files file.clone() is used multiple 
times. It is not clear how to fix that, maybe it is better to pass 
String instead of PathBuf

  - try_key_from_properties_files: I suggest to separate iteration and 
processing logic for each element into different functions

  - inconsistent indentation in try_jdk_from_properties and in tests


4. os_access

  - in get_user_config_dir and get_legacy_user_config_dir please use map 
to convert one Option into another


-- 
-Alex