-
Notifications
You must be signed in to change notification settings - Fork 98
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
Sorting of PDS members by date created #2707
Conversation
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2707 +/- ##
==========================================
- Coverage 93.26% 93.24% -0.02%
==========================================
Files 102 102
Lines 10544 10562 +18
Branches 2260 2286 +26
==========================================
+ Hits 9834 9849 +15
- Misses 709 712 +3
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
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, did leave a comment regarding clean code if we want to go that route but good approving in it's state. Thanks @SanthoshiBoyina for the new Functionality.
@@ -326,7 +328,30 @@ export class ZoweDatasetNode extends ZoweTreeNode implements IZoweDatasetTreeNod | |||
return sortByName(a, b); | |||
} | |||
|
|||
if (sort.method === DatasetSortOpts.LastModified) { | |||
if (sort.method === DatasetSortOpts.DateCreated) { |
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.
with the list growing for ways of sorting should we consider a switch/case in place of the continued if/else if's?
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
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 @SanthoshiBoyina
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 @SanthoshiBoyina
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 @SanthoshiBoyina - I left a comment regarding the changelog entry and had a small question about handling the "created date" (c4date
) property.
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
16620d4
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 enhancement and for addressing my feedback @SanthoshiBoyina 🙂
Quality Gate failedFailed conditions |
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! 😋
I'm curious what should we do about the member that don't have a c4date
and/or m4date
. Right now we fallback to sorting by name (which makes sense). The interesting part was that we use the same sort direction (descending). 🤔
I'm not suggesting we change it, it just surprised me to see a bunch of them without dates 😋
@SanthoshiBoyina make sure these changes are ported to the next branch, thanks 😄 |
Proposed changes
To add an additional PDS member sorting option that allows users to sort by Date Created.
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