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

[Jiri Vanek]

On 11/21/18 1:42 AM, Alex Kashchenko wrote:
> 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.

I realised the same. But I will do it as separate changeset, as it would not be refactoring if logic
is change dlike this.
> 
>  - 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).

I'm a bit afraid to do this in same changeset as this refactoring. But will gladly do so in separate
changeset.
> 
>  - 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)

ugh. Cant parse. Can you please provide example? Also see lower. Imho it do not belong to this
chnageset. And not sure how it is better.
> 
>  - please drop is_file, I suggest using path.metadata().map(|md|md.is_file()).unwrap_or(false) instead

Not sure how this is better. Even if followed (which is going to happen) I will keep it in is_file. ok?

> 
>  - 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

I think I need look closer to those maps. HAve some lack of knowlege here.
> 
>  - please drop verify_jdk_string, instead pass string ref to verify_jdk_path

Will do. BUt form curiosity - why?
> 
> 
>  - inconsistent validation in impl Validator line and in tests::get_jre_from_file

I cant see this:(
> 
> 
> 3. property_from_files_resolver
> 
>  - try_key_from_properties_files: please move string literals into format strings

Again, I think this should go as separate changeset. BUt we agreed  this write can be used. Why not
now? How is formated string better?
> 
>  - 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

What is wrong with clone? It i snice protection against unwonted change inside method. Anywa -
shoudl go as separate chnageset again.
> 
>  - 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

Thanx!.
> 
> 
> 4. os_access
> 
>  - in get_user_config_dir and get_legacy_user_config_dir please use map to convert one Option into
> another

Again maps. I need to look into them. Will do.
> 
> 


Generally - most of those issues shold be done as separate changeset. I really do not wont to spill
the refactoring. On contrary I agree with most of them.  So the adapted patch will cntain only minor
things (maps, is_file, dropped verify_jdk_path, formatting) ook?
Alsoplease explain the Validator, Cant see th eissue you do.

Thankx a lot for review!
  J.

-- 
Jiri Vanek
Senior QE engineer, OpenJDK QE lead, Mgr.
Red Hat Czech
jvanek at redhat.com    M: +420775390109