[pve-devel] [PATCH manager v2 1/2] ui/Parser: add generic functions property_strings
Dominik Csapak
d.csapak at proxmox.com
Thu Aug 2 15:56:52 CEST 2018
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
More information about the pve-devel
mailing list