-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented getting metadata for profiletypes #2471
Implemented getting metadata for profiletypes #2471
Conversation
Signed-off-by: Likhitha Nimma <[email protected]>
Codecov ReportAll modified lines are covered by tests ✅
📢 Thoughts on this report? Let us know!. |
Signed-off-by: Likhitha Nimma <[email protected]>
public setConfigArray(extendermetadata: zowe.imperative.ICommandProfileTypeConfiguration[]): void { | ||
extendermetadata?.forEach((item) => { | ||
this.profileTypeConfigurations.push(item); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the method is named setConfigArray
, I think it should replace the contents of the entire array. Since we are appending items to the array instead, perhaps it could be called updateConfigArray
or addToConfigArray
instead?
Also are we certain that this method won't be called more than once for the same profile type? Wondering if we should check to see whether an item already exists in the array before pushing a duplicate item (or we could use a Set
for this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled duplicate item. Made the required changes.
Signed-off-by: Likhitha Nimma <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment, @t1m0thyj raises a good concern
Signed-off-by: Likhitha Nimma <[email protected]>
5afd103
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 😋
let index = this.profileTypeConfigurations.findIndex((ele) => ele.type == item.type) | ||
if(index!=-1){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for being picky, and I know this does not have any negative effects in this particular situation, but should we use strict equality when comparing anything other than == null
?
I know this didn't show up in the Lint failures though, that's why I left the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Thanks for letting me know. Fixed it
profilemetadata.push(profilemetadata[0]); | ||
profCache.addToConfigArray(profilemetadata); | ||
expect(profCache.profileTypeConfigurations).toEqual(profilemetadata.filter((a, index) => index == 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clever way to test that the addToConfigArray
method is intended to avoid duplicates 😋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mentiong that there is only one lint error
/home/runner/work/vscode-extension-for-zowe/vscode-extension-for-zowe/packages/zowe-explorer-api/src/profiles/ProfilesCache.ts
[_eslint_] 89:17 error 'index' is never reassigned. Use 'const' instead prefer-const
Signed-off-by: Likhitha Nimma <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 😋
Thanks for the quick fixes! 🥳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment regarding addToConfigArray
- aside from that, LGTM!
Signed-off-by: Likhitha Nimma <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @likhithanimma1!
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @likhithanimma1 for this new API for extenders and for addressing all of the feedback.
Proposed changes
Fixed the issue of getting the meta data of profile types for creating the config files by adding the setters and getters in the zowe-explorer-api.
Release Notes
Milestone:
Changelog:
Types of changes
What types of changes does your code introduce to Zowe Explorer?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the revieweryarn workspace vscode-extension-for-zowe vscode:prepublish
has been executedFurther comments