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

[Stoiko Ivanov]

On Thu, 2 Aug 2018 15:56:52 +0200
Dominik Csapak <d.csapak at proxmox.com> wrote:

> 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

LGTM - would you commit it like that - or shall I send and update?

huge thanks btw. for the review and tips!
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel