[pve-devel] [PATCH v2 manager 2/4] node zfs: added new component to display additional zfs details

[Thomas Lamprecht]

On 11/6/18 1:48 PM, Tim Marx wrote:
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
>  www/manager6/node/ZFS.js | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/www/manager6/node/ZFS.js b/www/manager6/node/ZFS.js
> index c28e20c9..e2fa579d 100644
> --- a/www/manager6/node/ZFS.js
> +++ b/www/manager6/node/ZFS.js
> @@ -262,6 +262,46 @@ Ext.define('PVE.node.ZFSStatus', {
                                   ^^^^^^^^^^^^^^^^^^^

>      }
>  });
>  
> +Ext.define('PVE.node.ZFSStatus', {
               ^^^^^^^^^^^^^^^^^^^
Hmm your patch splitting is a bit strange to me...
You effectively define the same class two times here, only the later will prevail
and you have a "broken" GUI if building at this point.

In general each commit should be a change that can live on its own, does not
(temporarily) break anything and they should be causal, i.e., only
depend on their past history.

This is really important when doing bisecting, which gets really tedious if the
history contains such commits.
It'd make reviewing also much easier, I can much better process a single belonging
changes which do not depend on future changes to work than
a) multiple changes thrown together
b) changes wrongly split up so that a interwoven history gets made
c) (unnecessary) temporary breakage

Currently 2, 3 and 4 are interwoven, in 2 you have a class name conflict in 3 you
mix some addition with unexplained deletions and in 4 you only bring them together
so that the ZFSList class call the correct module again

could you please revisit patch 2, 3 and 4 regarding this (and trailing whitespaces ;))

You could maybe do them like:

* Keep the Status class name, only extend it with the read, write cksum properties
* Do the prepwork, add the new class in final form, maybe name it ZFSDevices not config,
  as it does not only shows the config but also the state of all devices set up.
* now switch over to the new panel showing both elements in PVE.node.ZFSList

The result looks great the code would be OK (besides whitespace errors), it's really
only the split up which confused me.

> +    extend: 'Proxmox.grid.ObjectGrid',
> +    xtype: 'pveZFSStatus',
> +    layout: 'fit',
> +    border: false,
> +
> +    initComponent: function() {
> +	 /*jslint confusion: true */
> +        var me = this;
> +
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +
> +	if (!me.zpool) {
> +	    throw "no zpool specified";
> +	}
> +	
    ^^^^
you add trailing whitespaces here, and in other places, please try to avoid this.

> +	me.url = "/api2/extjs/nodes/" + me.nodename + "/disks/zfs/" + me.zpool;
> +	
    ^^^^
trailing whitespaces..
> +	me.rows = {
> +	    scan: {
> +		header: gettext('Scan')
> +	    },
> +	    status: {
> +		header: gettext('Status')
> +	    },
> +	    action: {
> +		header: gettext('Action')
> +	    },
> +	    errors: {
> +		header: gettext('Errors')
> +	    }
> +	};
> +	
    ^^^^
trailing whitespaces..
> +	me.callParent();
> +	me.reload();
> +    }
> +});
> +
>  Ext.define('PVE.node.ZFSList', {
>      extend: 'Ext.grid.Panel',
>      xtype: 'pveZFSList',
>