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

[Alex Kashchenko]

Hi,

On 11/23/2018 03:15 PM, Jiri Vanek wrote:
> 
> [...]
>
>>
>> 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.

I mean to use something like:

write!(&mut info1, "itw-rust-debug: trying jdk over properties ({})",
         property_from_file::JRE_PROPERTY_NAME).expect("unwrap failed");

instead of:

write!(&mut info1, "{}", "itw-rust-debug: trying jdk over properties 
(").expect("unwrap failed");
write!(&mut info1, "{}", 
property_from_file::JRE_PROPERTY_NAME).expect("unwrap failed");
write!(&mut info1, "{}", ")").expect("unwrap failed");


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

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?

I think it is not doing anything useful to be in a separate function. 
Also in this function it probably should be &str instead of &String.


>>   - inconsistent validation in impl Validator line and in tests::get_jre_from_file
> 
> I cant see this:(

Sorry, this is a typo, it should be "incorrect indentation".


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

I added example above, I think I overlooked this for the previous patch.


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

I think this variable is not changed, so there should be not real need 
to clone it.


>
> [...]
>
> 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?

OK.

>
> [...]
> 


-- 
-Alex