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

[Jiri Vanek]

On 11/26/18 6:18 PM, Alex Kashchenko wrote:
> 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.
> 
>>
>> [...]
>>
> 
> 

Hello!

All should be fixed, unless it leadked to much form refactroing. todo:
 - home handling - will deffinitley land in next changeset
 - 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 :) )

ok?

The is still same, so  adapted patch for verbose from p[roeprties attacheched too.
/me jumping on $HOME now

TYVM!
  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: refactoredProperties2.patch
Type: text/x-patch
Size: 45336 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20181129/0a7082f4/refactoredProperties2-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: usingRefactoredProeprtiesToLoadVerboseFromSettings2.patch
Type: text/x-patch
Size: 4848 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20181129/0a7082f4/usingRefactoredProeprtiesToLoadVerboseFromSettings2-0001.patch>