I've spoken with Karen offline. We both agree there are pros and cons to
both of our perspectives.
Karen has agreed the code can stay as I have it as the differences are
stylistic.
Thank you again Karen!
Joe
On 03/25/10 06:59 PM, Karen Tung wrote:
> Hi Joe,
>
> Please see the response inline. I removed all the items that I have
> no further comments on.
>
> On 03/25/10 11:44, Joseph J VLcek wrote:
>> Hi Karen,
>>
>> The updated webrev can be found here:
>> http://cr.opensolaris.org/~joev/bug15163_r2/
>>
>> My comments are below.
>>
>> Thank you for the review.
>>
>> Joe
>>
>>
>> On 03/12/10 07:41 PM, Karen Tung wrote:
>>
>>
>>
>>>
>>> - line 244-303: The "invalid_argument_found" variable is defined. I
>>> don't think
>>> it is necessary. Since any invalid argument will cause the program to
>>> exit.
>>> Why not exit immediately? Why waste time to do other computations?
>>
>> This method will prevent the user from running multiple times to
>> identify all invalid arguments. For example, in the worse case, if all
>> 8 required and optional arguments are bad, this method prevents the
>> user from running once, being notified the 1st argument is bad,
>> rerunning again to be notified the next argument is bad... and so forth.
>>
>> This method notifies the user of all invalid arguments in a single run.
>
> My opinion actually differs from what you have. If we encounter the
> first invalid argument, we
> immediately exit and print the error, people be able to very easily see
> what's the problem, because that's
> the only problem that's reported.
>
> On the other hand, if we check everything else,
> the output reporting the errors will become long, and it will distract
> people from finding what actually is wrong. In addition, people
> sometimes just missed specifying one
> of the arguments which caused the whole list of arguments to be shifted.
> So, all the follow on "errors"
> might not be errors at all, and checking all of them is useless.
>
>>
>>
>>>
>>> - line 251-267: I think all these code is not necessary because those
>>> values
>>> are not used for installation at all.
>>
>>
>> Even though this script does not use them today if any of them are
>> invalid it could indicate a seriously wrong environment. Also if a
>> future code change requires more of these arguments they are already
>> checked.
>>
> That's true. All other finalizer scripts only access and validate the
> arguments it uses. If
> other variables need to be used in the future, they can be added. The
> start of all
> the finalizer script checks to make sure 5 arguments are passed. I
> believe that's
> enough to validate that we are running in a sane environment.
>
> Thanks,
>
> --Karen
