Skip to content

Add regex support to Search.dataSets#2443

Merged
zFernand0 merged 18 commits intozowe:masterfrom
JWaters02:feat/ds-search-regex-2
Mar 25, 2025
Merged

Add regex support to Search.dataSets#2443
zFernand0 merged 18 commits intozowe:masterfrom
JWaters02:feat/ds-search-regex-2

Conversation

@JWaters02
Copy link
Copy Markdown
Member

@JWaters02 JWaters02 commented Feb 22, 2025

What It Does

Closes #2432

Implements the regex option on the data sets search:

  • On mainframe search, it queries on ?research
  • On local search, it just uses normal regex matchAll

In #2432 I pondered about implementation of this option without causing a breaking change:

There are two options:

  1. Copy what searchJobs has, which means to make searchString into an optional parameter, then add a new optional parameter alongside it called searchRegex
  2. Keep searchString a required option, but add an optional boolean called isRegex or similar, which when true would call the API with the research query instead of the search one.

I like option 1 more because it would be a standard implementation between jobs and datasets. However, unfortunately this would cause a breaking change (as searchString would have to be made optional), which I would prefer to avoid (to allow this feature to come sooner), so therefore option 2 would have to do. But please let me know what your thoughts on that are.

No one let me know what their thoughts on that were, but I think it is pretty straight forward that option 2 is the right way to go for v3. So what I implemented was a new, optional --regex option (please let me know if the name should be changed) that when specified sets the searchString into a regex one. If reasonable, I could open an issue with the code changes required to switch to option 1 that would go in as v4 breaking change.

Obviously, one thing that needs to be taken into account, is that cases is not sensitive by default, which if combined with a regex string, may produce unintended effects - but that is up to the user (and the docs to point it out). For example, this regular expression specifies only upper case characters, but since the --case-sensitive option is not passed, it will match on the lower case characters anyway:

image

I had to refactor the logic in Search.dataSets and Search.searchLocal functions to take into account that the matched string length, for the text highlighting in output, would not be the same as the search string length for a regex search string.

Here are some random examples of using it:

image

How to Test

Run zowe files search data-sets <pattern> <search string> --regex and mess around with it!

Review Checklist
I certify that I have:

Additional Comments

This will require a PR on the docs-site so go in alongside it.

Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
@JWaters02 JWaters02 force-pushed the feat/ds-search-regex-2 branch from 578629d to eb16bee Compare February 22, 2025 20:55
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.53%. Comparing base (d6e4f06) to head (3bf06d9).
Report is 20 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2443   +/-   ##
=======================================
  Coverage   91.53%   91.53%           
=======================================
  Files         641      641           
  Lines       18444    18454   +10     
  Branches     3901     3906    +5     
=======================================
+ Hits        16882    16892   +10     
  Misses       1560     1560           
  Partials        2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
@JWaters02
Copy link
Copy Markdown
Member Author

I couldn't figure out how to run the system tests for the Search.system.test.ts file, and docs isn't clear enough on how to set it up, so I got really confused and gave up. So that file is currently untouched. Any help on how to get it working would be muchly appreciated.

@JWaters02
Copy link
Copy Markdown
Member Author

The build CI is failing because the snapshot has changed, how do I commit that?

Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
@zFernand0
Copy link
Copy Markdown
Member

zFernand0 commented Feb 26, 2025

The build CI is failing because the snapshot has changed, how do I commit that?

Hey @JWaters02 👋

You could run npx jest packages/cli/__tests__/zosfiles/__integration__/search/ds/cli.files.search.ds.integration.test.ts -u 😋

Then just push the corresponding snapshots which should be properly updated 😋

Copy link
Copy Markdown
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 😋

Thanks for implementing this feature. 🙏
Happy to approve once the build errors are resolved (snapshots updated) 🥳

Copy link
Copy Markdown
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JWaters02 for your contribution! Left a few comments

I agree the 2nd option you picked is the better one, at least for now in V3 to avoid breaking changes

Comment thread packages/zosfiles/CHANGELOG.md Outdated
Comment thread packages/zosfiles/src/methods/search/Search.ts Outdated
zFernand0 and others added 4 commits March 7, 2025 06:38
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
@JWaters02
Copy link
Copy Markdown
Member Author

Tests failed for the same snapshot reason, but I comitted the updated snapshot already, so not sure what the issue is now.

@JWaters02 JWaters02 requested review from t1m0thyj and zFernand0 March 7, 2025 16:09
@awharn
Copy link
Copy Markdown
Member

awharn commented Mar 10, 2025

Tests failed for the same snapshot reason, but I comitted the updated snapshot already, so not sure what the issue is now.

A recent change was made on the master branch that changed how line wrapping works in the CLI help. It appears the snapshots were based on the old line wrapping, and committed just after the new wrapping was merged into this branch.

I believe that another snapshot update is required, with the newest version of the CLI built and installed.

Additionally, if feasible, it would be nice to have a system tests to confirm the functionality is working.

Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
@JWaters02
Copy link
Copy Markdown
Member Author

Additionally, if feasible, it would be nice to have a system tests to confirm the functionality is working.

Hi Andrew, I tried to set up the default_properties.yaml with as much detail as I could provide for the zxplore system, but I think I am simply missing a lot of permissions on that system to be able to run the tests, as most of them are failing due to errors on the zosmf server. I also wouldn't want to burden the zxplore server so probably best for me to leave it.

I can't test on my work system, because I am only able to do development on my home PC, which does not have the VPN that can access the work mainframe.

I agree that system tests would be good, but I am just not able to do it, so best if someone else did.

Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Copy link
Copy Markdown
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 😋

Not sure how to feel about the system test 😅
But, I believe we can get this one for now and circle back later. 🙏

Copy link
Copy Markdown
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and changes LGTM, thanks @JWaters02!

@zFernand0 zFernand0 dismissed t1m0thyj’s stale review March 24, 2025 14:59

It'd seem that the requested changes were addressed 🙏

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @JWaters02!

@zFernand0 zFernand0 merged commit eef087b into zowe:master Mar 25, 2025
21 checks passed
@github-project-automation github-project-automation bot moved this from Review/QA to Closed in Zowe CLI Squad Mar 25, 2025
@zFernand0 zFernand0 added the release-minor Indicates a minor feature has been added label Mar 25, 2025
@github-actions
Copy link
Copy Markdown

Release succeeded for the master branch. 🎉

The following packages have been published:

  • npm: @zowe/imperative@8.16.0
  • npm: @zowe/cli-test-utils@8.16.0
  • npm: @zowe/core-for-zowe-sdk@8.16.0
  • npm: @zowe/zos-uss-for-zowe-sdk@8.16.0
  • npm: @zowe/provisioning-for-zowe-sdk@8.16.0
  • npm: @zowe/zos-console-for-zowe-sdk@8.16.0
  • npm: @zowe/zos-files-for-zowe-sdk@8.16.0
  • npm: @zowe/zos-logs-for-zowe-sdk@8.16.0
  • npm: @zowe/zosmf-for-zowe-sdk@8.16.0
  • npm: @zowe/zos-workflows-for-zowe-sdk@8.16.0
  • npm: @zowe/zos-jobs-for-zowe-sdk@8.16.0
  • npm: @zowe/zos-tso-for-zowe-sdk@8.16.0
  • npm: @zowe/cli@8.16.0

Powered by Octorelease 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-minor Indicates a minor feature has been added released

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

Add regex support to Search.dataSets

5 participants