Re: [pgadmin-hackers] Feature test regression failures - Mailing list pgadmin-hackers
| From | Atira Odhner |
|---|---|
| Subject | Re: [pgadmin-hackers] Feature test regression failures |
| Date | |
| Msg-id | CA+Vc24p6u5XNYFgrF4uzAeqgjxVkceGpre=iHESiyRABC=+-zQ@mail.gmail.com Whole thread Raw |
| In response to | Re: [pgadmin-hackers] Feature test regression failures (Ashesh Vashi <ashesh.vashi@enterprisedb.com>) |
| Responses |
Feature test regression failures
(Dave Page <dpage@pgadmin.org>)
|
| List | pgadmin-hackers |
Hi Ashesh,
First - let me try to explain the problem with the failure in the feature test.
We do not load all the javascript libraries, when starting the pgAdmin 4 (i.e. the loading the browser/index.html).
But - load them only when first tree-item of certain type is added.
e.g. We load the javascript module of 'table', 'index', etc only after first tree item of database is added in the tree.I've seen this bug opening the tree by hand, so it is not merely a matter of opening tree items as quickly as a machine. There are two different code paths for loading the tree state --- the way the initial load happens upon expand and the call that is made when the user clicks 'Refresh...' in the context menu. Maybe a good approach to starting to fix this bug would be to have only one code path for loading tree state.What do you mean by that?
I don't think this has to do with loading the javascript modules.It is - we've personally experienced the same in early phase of the development, when the spinner control was not implemented.We have seen that - when we expand the tree node, it does reloads the server-groups under the server-group node, just because of the same reason.
I understand that the javascript modules are being loaded dynamically when a corresponding item is first revealed in the tree, and although that seems unnecessary, I still don't think that is the cause of this bug. I think it is far more likely to be a result of sharing state that should not be shared. I am having a hard time tracking down the bug because I find the code very difficult to read largely because of the one-letter variable names and lack of meaningful isolation between code paths.
But - as I said earlier, it represents the data for the tree-item (not the item itself).
Hence - it should be 'itemData', and not 'item'.'itemData' and 'item' are both fine with me.We've already used 'd' to represent the data, 'i' for item, 't' for tree at all other places in pgAdmin 4.
So - it is consistent across the application.Consistency is great, but in this case, aiming for consistency is hindering the legibility of the application code. At some point, we should go through the whole application and change all the 'i' to 'item', 'd' to data', etc. Until then, I think it is worthwhile to sacrifice consistency in exchange for adding meaning and clarity to the code that is being updated.To be honest, not in that case.As the current choice of the alphabet tells enough about the meaning of the code.
Clearly if I'm renaming it with the wrong thing, it isn't enough for me to tell about the meaning of the code.
We're deviating from the actual point at the moment, so - let's concentrate on that first.I would change it to itemData.
Great. Please change it to itemData.
Thanks,
Tira & Joao
pgadmin-hackers by date: