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

[Thomas Lamprecht]

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)

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

> +    },
> +
>      parseQemuNetwork: function(key, value) {
>  	if (!(key && value)) {
>  	    return;
>