[pve-devel] [PATCH manager v2 1/2] ui/Parser: add generic functions property_strings
Thomas Lamprecht
t.lamprecht at proxmox.com
Thu Aug 2 15:52:00 CEST 2018
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?
More information about the pve-devel
mailing list