On 11/6/18 1:48 PM, Tim Marx wrote:
> Signed-off-by: Tim Marx <[email protected]>
> ---
> 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',
>
_______________________________________________
pve-devel mailing list
[email protected]
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
