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

[Thomas Lamprecht]

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

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