[rfc][icedtea-web][policyeditor] Parsing enhancements and unit tests

[Jiri Vanek]

On 03/08/2014 05:07 AM, Andrew Azores wrote:
> Well, I guess I could make one matcher, check if it matches, and then if not, make the other and check with it, but how much does it really matter... ? I think the intention is clearer, having the conjunction in the first if check. I doubt it will make much of a perfomance difference for most users,
 > unless they end up with much more massive custom policy files than I'm expecting.

Well its not about the performance of this method, but about ITW as a whole. Consider such a redundant calls in each method. They will mutliply runtime later unbearable.
>
> And of course the CustomPermission and PolicyEditorPermissions fromString() methods are "suspiciously" similar

Then why they dont share an code?

 > - they are both reading from the same source and attempting to parse into more or less the same end result. Why would they be really different? I guess the logic is mostly the same so they could be given a parent class or some utility method.

Please eleaborate on this two hunks a bit.

>
> Thanks,
>
> Andrew A
>
> ----- Original Message -----
> From: "Jiri Vanek"<jvanek at redhat.com>
> To: "Andrew Azores"<aazores at redhat.com>
> Cc: "IcedTea"<distro-pkg-dev at openjdk.java.net>
> Sent: Friday, March 7, 2014 8:24:09 AM
> Subject: Re: [rfc][icedtea-web][policyeditor] Parsing enhancements and unit tests
>
> The messages.properties changes are still not listed in changelog.
and dont forget^ O:)

>
> This is probably nit, but:
> +        final Matcher targetMatcher = PolicyEditorPermissions.TARGET_PERMISSION.matcher(string);
> +        final Matcher actionMatcher = PolicyEditorPermissions.ACTIONS_PERMISSION.matcher(string);
> +        if (!targetMatcher.matches()&&  !actionMatcher.matches()) {
> +            return null;
> +        }
> +
> +        final String typeStr, targetStr, actionsStr;
> +
> +        if (actionMatcher.matches()) {
> +            typeStr = actionMatcher.group(1);
> +            targetStr = actionMatcher.group(2);
> +            actionsStr = actionMatcher.group(3);
> +        } else {
> +        	typeStr = targetMatcher.group(1);
> +            targetStr = targetMatcher.group(2);
> +            actionsStr = "";
>            }
>
> the first matcher is needed only when second one fails. Maybe would be nice to remove the
> unnecessary matching.
>
> similar for PolicyEditorPermissions which is still suspiciously similar:(
>
>
> The chnage [\\w.] ->  [\\w\\.]  looks enough :)
>
>
>
> Thanx!
> J.
> On 03/06/2014 09:18 PM, Andrew Azores wrote:
>> Thanks for review! Hopefully the new attached patch is nicer.
>>
>> Thanks,
>>
>> Andrew A
>>
>> ----- Original Message -----
>> From: "Jiri Vanek"<jvanek at redhat.com>
>> To: "Andrew Azores"<aazores at redhat.com>
>> Cc: "IcedTea"<distro-pkg-dev at openjdk.java.net>
>> Sent: Thursday, March 6, 2014 9:07:46 AM
>> Subject: Re: [rfc][icedtea-web][policyeditor] Parsing enhancements and unit tests
>>
>> On 03/05/2014 06:01 PM, Andrew Azores wrote:
>>> Hi,
>>>
>>> The previous thread was diverging into two quite different patches, so I'm splitting it.
>>>
>>> This patch improves parsing, especially handling of comments, and adds a lot of new unit testing for this. A few small bugs were found and fixed along the way.
>>>
>>
>> The Messages.properties changes are missing in in-pathc chagelog
>>
>> public static CustomPermission fromString - more more and more tests. Try to hack yourself.  But generally ok.
>> Well the comment is poem-writer's masterpiece :) But better then nothing :)
>> The [\\w.] do not osund correct. The dot should be escaped to match dot, not any char. Or not? So this part will be:
>> [[\\w\\.]+\\w+] (nottested - to match [java.][io.][permissions] and nto eg dsgfgogfdsgjsfdghjsfd[hj as now :)
>> The content of quotes may be anything, ok? What about content to be qutes itself? (not jsut ${} eg?
>> The first quotes are mandatory, and the scond not? As far as I read from regex.
>> Why is compiled patter not global constant? It should be. It will not need recompile each launch time... Then there can be also some tests only to the regex itself.
>>
>>
>> Why is the  CustomPermission.fromStirng copypasted to PolicyEditorPermissions ???
>>
>> Quick glance over tests is ook.
>>
>> Thanx!
>>      J.
>>
>