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

[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

in the case of parseXXX the caller is responsible to check
if the returned value is an object or undefined
and handle a parse error (be it a msg box or something else)

> 
>> +	}
>> +
>> +	return res;
>> +    },
>> +
>> +    printPropertyString: function(data, defaultKey) {
>> +	var stringparts = [];
>> +
>> +	Ext.Object.each(data, function(key, value) {
>> +	    var keystring = '' ;
>> +	    if (key === defaultKey) {
>> +		keystring = '';
> 
> key string is set to '' above, so a single branch with
> 
> if (defaultKey === undefined || key !== defaultKey)
> 
>> +	    } else {
>> +		keystring = key + '=';
>> +	    }
>> +	    stringparts.push(keystring+value);
>> +	});
>> +
>> +	return stringparts.join(',');
> 
> AFAIK, defaultKey must be a start of serialized propertyString?
> So you may want to ensure that this holds...

no, parse_property_string in jsonschema does not care about the order
although when writing to the config, it does write the defaultkey first

> 
>> +    },
>> +
>>       parseQemuNetwork: function(key, value) {
>>   	if (!(key && value)) {
>>   	    return;
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>