Context Value generation refactoring

[stepanzharychevbroadcom]

As well as with issue #480 we have a problem with extending of things which are based on context value. Currently it's used only for specifying of favourite, but in the future we may need to operate with this one more. So we should figure our how to bring similar strategy pattern into this logic.

[stepanzharychevbroadcom]

After reviewing of the code I found out that we have pretty problematic class structure, so even before we start with Context Value generator itself something should be figured out about this.

Based on what's represented on Chart №1 you can see that for absolutely different needs we use similar classes, so far it results in just 3 of those:

• ZoweNode;
• ZoweUSSNode;
• ZoweJobNode;

As you can imagine those classes have lots of unnecessary methods/properties in their interface (e.g. all of them have method getChildren even if semantically there can't be any of those). The worst of such side effects is how we define context value for those classes using if-else condition on the level of constructor and separate parameter to do override. To my mind such approach leads directly to hardly readable and usable code as a result. So we should start from splitting of those generic classes onto logical parts. Suggested way is represented on Char №2:

So comments to the approach above are:

• Properties means which of the class properties have an influence on generated Context Value;
• Each of classes will be better documented later - once we start with implementation;
• Groups mean how we can split work on those modules, so we don't need to do all at once;

Another thing which caused the questions is represented on Chart №3, so it's about how we do dependency injection for Session/Folder/Memeber groups, I guess that chart is pretty self talking, but still the explanation is:

• Session gets only Profile dependency injected;
• Folder gets Session as dependency (and as a result has an access to Profile);
• Member gets both Session and Folder as dependencies;

The plan for implementation will include 3 (or may be more) steps, so we do Groups one by one, I would suggest to begin with №2 or №3, because they're much smaller and we can practically see the way of implementation. Along with Group implementation we can start writing the Context Value generator, which will produce Context Value based on Class and properties of its instance.

@Colin-Stone @jellypuno @jalel01 You're welcome for further discussion.

[zFernand0]

As a general comment, I do agree that the Strategy Pattern is probably the best approach when dealing with runtime behavior.

If I may, I wanted to present the contrast of using Interfaces instead of Classes (as I've seen the idea of creating a good amount of classes in the past few UML diagrams).

Most OOP languages take advantage of classes as it lets developers define behaviors (groupings, namespaces, ...), as well as provide said behavior/implementation/function within the class itself. In Typescript, interfaces are used to outline properties and functions that need to added by the object that implements it.

I understand I could be completely out of line here and may be missing very important details about the actual implementation of this refactoring discussions. My apologies for this. I promise to follow this more closely.

P.S. I'm not against of the overall intent of reorganizing the current code structure. P.P.S. I would love to suggest basing the diagrams off of the Inter2 branch if possible next time as it contains some amazing stuff : )

[jellypuno]

I am still trying to understand this so I will ask in a different way.

Currently if I click search in VSCode the process will be

1. extension.ts vscode.commands.registerCommand("zowe.pattern", (node) => datasetProvider.datasetFilterPrompt(node));
2. Something with getChildren()
3. DatasetTree.ts public async datasetFilterPrompt(node: IZoweDatasetTreeNode)

All the information that I need will be under the variable node including context value, session, profileName etc.

I like what @Colin-Stone did for the IZoweTreeNode because all common values are combined in that node and if USS, Jobs or dataset have a different one then I can just expand that interface.

So, if you implement this how will this variable node will look like? How will it be separated to PSNode, PDSNode, DSSessionNode if I haven't started searching the dataset yet? Is this architecture focused only in contextValue?

[jellypuno]

I would love to suggest basing the diagrams off of the Inter2 branch if possible next time as it contains some amazing stuff : )

@zFernand0 I agree. If we are going to re-architect the code we should combine all ideas regarding refactoring so we could have a bigger picture of the approach. For me, an architecture call is really important. Tagging @jalel01 @Alexandru-Dumitru @crawr

[Colin-Stone]

As well as with issue #480 we have a problem with extending of things which are based on context value. Currently it's used only for specifying of favourite, but in the future we may need to operate with this one more. So we should figure our how to bring similar strategy pattern into this logic.

context value is key to the way VSCode links entities defined in package.json with entities in the code itself. package.json controls the menus, visibility of these entities and is the closest we have to extensibility like Eclipse. That doesn't mean to say I don't think it could be improved or our code could be refined. My interest here is about further extensibility by third parties and so I would avoid any enhancement that may reduce that capability.

[Colin-Stone]

Please consider that the Zowe Explorer has taken off in popularity in a very small amount of time and was until recently a sample, proof of concept originally demonstrating the versatility of the CLI. It was initially three separate extensions provided by three different people/groups that have been merged and gradually refined which will explain some of the reasons that some of the code structure is not ideal. The process of merging these three extensions into one is ongoing and the next phase was to tease out the current interfaces we are supporting, hence https://github.com/zowe/vscode-extension-for-zowe/issues/328 This should be reviewed and we should consider other aspects of legacy debt that has been building including full pipeline support and code maintainability. There is a lot to do and I feel we have been in catch up mode for a long time trying to balance new function with fixing. Besides considering an architecture call I think it would be good for everyone to consider the direction we should be taking with the Zowe Explorer as that could shape future architectural decisions

[stepanzharychevbroadcom]

@Colin-Stone @jellypuno I would say we need a call here to merge all of the ideas together and potentially add more classes for different DS/USS/Jobs. Thanks for a feedback guys!

[jalel01]

These are all great comments. I do feel like we should be more explicit with regard to identifying, documenting and planning technical debt into our releases. The extended 1 hour session on Fridays was intended as an opportunity to discuss technical debt, refactoring, rearchitecting etc. Please speak up if you feel that we should find another time slot.

Now that we are tagging issues as technical debt i think we can use fridays to reach a common understanding and then add them to release milestones according to an agreed upon priority.

[Colin-Stone]

@Colin-Stone @jellypuno I would say we need a call here to merge all of the ideas together and potentially add more classes for different DS/USS/Jobs. Thanks for a feedback guys!

I don't believe we require more classes. Although your diagram provides a valuable perspective of the z/OS environment the Zowe Explorer is but a thin client view of it, centred on particular functionality. Replication of every possible type of object is overkill. We are in the process of implementing interfaces and we may prefer to consider the use of Interface types to distinguish explicit types of objects that require alternative handling but equally for the level of differentiating we need to make a simple field value may be all that is required

Unless I have missed something in my understanding ContextValue is non-negotiable as it is required by VScode hardcoded in its string form in package.json. Although there is a benefit in replacing the mechanism created to manage favorites and icons with an alternative, I don't regard this as broken and is reasonably well understood.

[stepanzharychevbroadcom]

@Colin-Stone Just to make it clear for me: you're opposite to the idea of splitting those generic classes on functional parts?

Not entirely. I am with you for folder, file, session etc but not down to explicit types like PDSE, DSNode etc. Group 2 and 3 would be fine.

I think it is necessary to have a different policy for these

[Colin-Stone]

Sorry I updated your comment some how. I don't know what the best way would be to split the function out. Maybe it's by a folder/file/session explicit class, a strategy based on the interface type or maybe using generics and different interface types also.

[Colin-Stone]

Also I would like to think how we would handle dataset alias types migrated files etc. These have alternative visuals and available commands and may warrant additional calls. They could be further specializations on dataset file but could also be the same as favorite

[Colin-Stone]

Thisis how I see your diagrams in my mind

[stepanzharychevbroadcom]

@Colin-Stone Ah, I see now. Okay, that makes sense. I guess we really need a meeting to discuss it. If we can merge some datasets it would help. I was mainly thinking about classes as a way of self-determination for tree entries so we can further extend them later without problems.

[stepanzharychevbroadcom]

@Colin-Stone Further explanation of how we're going to use classes for Context Value generation.

First of all we don't touch package.json it will stay hardcoded, we will just optimise how classes receive context value internally to prevent hardcode and make classes' state define context value not opposite.

On №1 you can see that class member is going to pass itself into the generator on certain actions and receive context value back, so class won't directly define which context value it has, only implicitly via type and properties.

On №2 you can see suggestions about generation rules. Example for such generation can be: 1) We generate for PS Dataset which is favourite, loading(example of extension-related state), migrated; 2) Main part will be pds; 3) Prefix part will be migr; 3) Suffix part will contain 2 suffixes: [loading, fav]. We sort them in ascending order, join and get the the following result: _favloading 4) Result: _migr_pds_fav_loading_;

Doing like that should make our life much easier.

[Colin-Stone]

Thanks Stepan. I love the dynamic generation techniques as it provides great flexibility to expand the objects that can be handled. I just stay a little "hung-up" about the fact that the range of contextValues used are statically defined in package.json. I appreciate you saying we don't touch them in package.json ;-) but the critical thing is that if we generate the contextValue and it responds to every possibile action/status the resulting generated contextValue needs to be something already defined in package.json to enable context menus etc to work. If you agree we are in synch. We then get the problem of the package.json file becoming huge because we have introduced so many subclasses that subtley alter the contextValue and we need to manually specify context menus etc for. The following is such and example, viewItem corresponds to contextValue