[pve-devel] [PATCH manager v2 1/2] ui/Parser: add generic functions property_strings

[Dominik Csapak]

On 08/02/2018 03:52 PM, Thomas Lamprecht wrote:
> Am 08/02/2018 um 03:29 PM schrieb Dominik Csapak:
>> On 08/02/2018 03:13 PM, Thomas Lamprecht wrote:
>>> Am 08/02/2018 um 02:51 PM schrieb Dominik Csapak:
>>>> On 08/02/2018 12:04 PM, Thomas Lamprecht wrote:
>>>>> Am 08/01/2018 um 08:29 PM schrieb Stoiko Ivanov:
>>>>>
>>>>> great for tackling this! some comments inline..
>>>>>
>>>>>> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
>>>>>> ---
>>>>>>     www/manager6/Parser.js | 43
>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 43 insertions(+)
>>>>>>
>>>>>> diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
>>>>>> index 13dce766..df2be5dc 100644
>>>>>> --- a/www/manager6/Parser.js
>>>>>> +++ b/www/manager6/Parser.js
>>>>>> @@ -45,6 +45,49 @@ Ext.define('PVE.Parser', { statics: {
>>>>>>                value === 'true';
>>>>>>         },
>>>>>>     +    parsePropertyString: function(value, defaultKey) {
>>>>>> +    var res = {},
>>>>>> +    errors = false;
>>>>>
>>>>> please indent errors here, else it may seem like errors is already
>>>>> defined (e.g., as global variable) a be a bit confusing...
>>>>> Do:
>>>>> var res = {},
>>>>>        errors = false;
>>>>>
>>>>>> +
>>>>>> +    Ext.Array.each(value.split(','), function(p) {
>>>>>> +        var kv = p.split('=', 2);
>>>>>> +        if (Ext.isDefined(kv[1])) {
>>>>>> +        res[kv[0]] = kv[1];
>>>>>> +        } else if (Ext.isDefined(defaultKey)) {
>>>>>> +        if (Ext.isDefined(res[defaultKey])) {
>>>>>> +            errors = true;
>>>>>> +            return false; //break
>>>>>> +        }
>>>>>> +        res[defaultKey] = kv[0];
>>>>>> +        } else {
>>>>>> +        errors = true;
>>>>>> +        return false; // break
>>>>>> +        }
>>>>>> +    });
>>>>>> +
>>>>>> +    if (errors) {
>>>>>> +        return;
>>>>>
>>>>> Hmm, maybe throw an exception? the above cases suggest a situation
>>>>> which
>>>>> continuing with and empty result may not be desired.
>>>>> You may want to omit the errors variable and just throw directly above.
>>>>>
>>>>> throw "there can only be one defaultKey"
>>>>> throw "invalid property string"
>>>>>
>>>>> (as suggestion)
>>>>
>>>> all parsers in Parser.js do this and it does not make sense to throw
>>>> an exception here, because in that case the user never notices
>>>> except that the code stopped working
>>> sounds like quite a big notification that something is not OK ;-)
>>> IMO, a property string has this hard asserts as it get passed from the
>>> backend, if those do not hold it like an BUG_ON in the kernel, and
>>> execution _should_ be stopped.
>>
>> i agree, but it should be stopped in an obvious way and not only
>> a log in the browser console, nobody has open normally
>> also, depending on where we parse, we might want to clean something
>> up/close a window/etc. and so far we do not use
>> try {} catch {} at all
>>
> 
> yes, but we also do not use this method yet (or only once if 2/2 from
> this manager series would be applied)
> 
>> a user reporting the msgbox 'could not parse drive options' is very
>> useful us, since we instantly know that the drive parser in the gui
>> is out of sync with the backend
>>
>> a user reporting 'editing a disk does not save values' is really subtle
>> since this can be many things
>>
> 
> I mean you can always ask user stuff, and only helps if the case gets
> catched by the caller in the first place, but yeah I get your point...
> 
> maybe a PVE.throw could be somewhat useful, showing an error popup and
> throwing up the error...

yes i would like that

> 
>> so far the only places we throw an exception (afair) are when
>> using components to make sure all necessary parameters are set
>> e.g. when it needs a vmid or nodename
>>
> 
> i.e., static vs. dynamic triggerable stuff.
> 
> Anyway, I'd apply this with a followup alike:
> 
> ----8<----
> diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
> index df2be5dc..99d06f6d 100644
> --- a/www/manager6/Parser.js
> +++ b/www/manager6/Parser.js
> @@ -47,7 +47,7 @@ Ext.define('PVE.Parser', { statics: {
> 
>       parsePropertyString: function(value, defaultKey) {
>          var res = {},
> -       errors = false;
> +           error = undefined;
> 
>          Ext.Array.each(value.split(','), function(p) {
>              var kv = p.split('=', 2);
> @@ -55,17 +55,18 @@ Ext.define('PVE.Parser', { statics: {
>                  res[kv[0]] = kv[1];
>              } else if (Ext.isDefined(defaultKey)) {
>                  if (Ext.isDefined(res[defaultKey])) {
> -                   errors = true;
> -                   return false; //break
> +                   error = 'defaultKey may be only defined once in
> propertyString';
> +                   return false; // break
>                  }
>                  res[defaultKey] = kv[0];
>              } else {
> -               errors = true;
> +               error = 'invalid propertyString, not a key=value pair
> and no defaultKey defined';
>                  return false; // break
>              }
>          });
> 
> -       if (errors) {
> +       if (error !== undefined) {
> +           console.error(error);
>              return;
>          }
> 
> 
> --
> 
> So no throw, but a guaranteed log to console with call trace.
> Would that be OK for you?
> 

yes that seems good