Skip to content
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

Enhance Object Browser filter's Object field #1780

Merged
merged 18 commits into from
Jan 29, 2024
Merged

Conversation

sebjulliand
Copy link
Collaborator

@sebjulliand sebjulliand commented Jan 14, 2024

Changes

Resolves #1779

This PR enhances the Object browser filters by adding support for more advanced filtering options.

Multi level filtering

Every field of a filter now supports filtering:

  • Libraries
  • Objects
  • Object types
  • Members
  • Member types

Mixed browsing

Multiple libraries can now be browsed with a single filter.
image

When source files are found in the list (if Object Types list includes *SRCPF or *ALL), they can be browsed as well - they are not treated as simple Objects anymore:
image

Filtering type

The filtering type is defined for every field of the filter.
image

  • Simple: each filed will support multiple comma-separated values that can contain multiple * as wildcard. Example: QSYS*, *TEST, *SAMPLE*, A*B*C, YAY
  • Regex: each field will support a single RegEx for filtering values

API: new getQTempTable content method

This method allows to prepare a file in QTEMP using multiple SQL statements and get the result. This allows for not relying on temporary tables written in the temporary library defined in the configuration.

content.getTable has been replaced by content.getQTempTable where relevant.

Tests

Additional unit tests have been provided to test the filter engine and the library/object/member methods

Checklist

  • have tested my change
  • for feature PRs: PR only includes one feature enhancement.

@sebjulliand sebjulliand added enhancement New feature or request build Build will be available inside PR. labels Jan 14, 2024
@sebjulliand sebjulliand requested a review from a team January 14, 2024 16:15
@sebjulliand sebjulliand self-assigned this Jan 14, 2024
Copy link
Contributor

👋 A new build is available for this PR based on a1f929c.

Copy link
Contributor

👋 A new build is available for this PR based on a249672.

Copy link
Collaborator

@chrjorgensen chrjorgensen left a comment

Choose a reason for hiding this comment

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

@sebjulliand As always a great enhancement from your hands! 😍

I have some comments besides the suggestions in the code:

  • Previously *ALL was allowed as object name and returned all objects (like a single *). This does not work anymore. I guess that is okay, since the description says a single * will do the trick.
  • When I specify a generic object name like *C* in the filter, nothing is returned. I guess your generic-test does not handle this? Maybe a regex test could solve this?

You know what's coming next... Can we have the same for members?! 🤣
Seriously, I'm convinced the users will ask for this - now they're given the option to have multiple objects in a filter.

We're still a bit behind RDi ("shh, don't mention the war!" 😆) in our filter options - in RDi you can specify a generic source file name linked to a generic member name and type, and have multiple of these groups in a filter. When I was thinking of coding this, that was my intention - to turn the object, member and type into a list, so the three would be linked together and had all to be met to be included in the output. This allows for more granular selection...

WDYT?

But still, a great enhancement...!!! 😍

@sebjulliand
Copy link
Collaborator Author

@sebjulliand As always a great enhancement from your hands! 😍

I have some comments besides the suggestions in the code:
...
WDYT?

But still, a great enhancement...!!! 😍

Thank you so much @chrjorgensen ! As always, your review is on point.
I'll look into the suggested changes and add support for xxx type of filter.

And then I'll apply the filtering logic to members and types; it should be fairly easy.

Co-authored-by: Christian Jorgensen <[email protected]>
Copy link
Contributor

👋 A new build is available for this PR based on 322ce5f.

@worksofliam
Copy link
Contributor

@sebjulliand Since you are enhancing getObjectList, I would really love to see some additional tests to support these changes

@sebjulliand
Copy link
Collaborator Author

@sebjulliand Since you are enhancing getObjectList, I would really love to see some additional tests to support these changes

Sure thing! Since I may update the library/object/member listing altogether, I'll try to write some tests for everything.

@jkyeung
Copy link

jkyeung commented Jan 18, 2024

First, thank you for this enhancement. It truly transforms the usability of the filter.

Second, I want to amplify the point about the members: If I could only choose to have "advanced" filtering on objects or members (I realize this is not a true choice, just trying to illustrate), I would choose members. In any shop that uses source physical files, the members are where all the most important naming conventions happen. Everyone mainly sticks to the object names provided by IBM (QRPGLESRC, QCLSRC, etc.), and there are not too many of them. But there are usually hundreds or even thousands of members in a given source file, and every shop has their own naming system for them.

Finally, to that end, I think a regex option would be extremely useful, but only for those users who are comfortable with regex. It would be confusing to many users. So if it's too much trouble to handle multiple styles of wildcard, I would stick to the basics: either just something equivalent to regex's .* (like the current * most IBM i users are used to, or % in SQL's LIKE predicate), or that plus something equivalent to regex's . (like SQL's _). Those would cover most cases for most users.

I could imagine a setting, where you can choose regex or "basic IBM" or "SQL-style", especially if you're going to implement any of them using regex under the covers anyway.

@sebjulliand
Copy link
Collaborator Author

Thanks for the feed @jkyeung !
I'm actually going to add support for regex in there, at every level (library, file and member) - we should end up with the same filtering ability as in RDi, but with regex support.

By default, filters will only support the * wildcard to keep it simple for most use cases and not confuse people. A checkbox in the filter definition will allow to turn on RegEx support if needed.

@jkyeung
Copy link

jkyeung commented Jan 19, 2024

I'm actually going to add support for regex in there, at every level (library, file and member)

I can't wait!

By default, filters will only support the * wildcard to keep it simple for most use cases and not confuse people. A checkbox in the filter definition will allow to turn on RegEx support if needed.

Perfect!

Otherwise there is an additional garbled row returned when running multiple queries at once.

Signed-off-by: Seb Julliand <[email protected]>
Signed-off-by: Seb Julliand <[email protected]>
Copy link
Contributor

👋 A new build is available for this PR based on cf0cd8c.

@sebjulliand sebjulliand requested review from chrjorgensen and a team January 24, 2024 15:22
@sebjulliand
Copy link
Collaborator Author

@codefori/core , you're on!
I updated the PR description to match what's done here. Let me know how it goes! 😄

Signed-off-by: Seb Julliand <[email protected]>
Copy link
Contributor

👋 A new build is available for this PR based on 14706b1.

Copy link
Contributor

👋 A new build is available for this PR based on 8887227.

@chrjorgensen
Copy link
Collaborator

@sebjulliand 🚀 🚀 🚀 🚀 🚀 - 5 out of 6 rockets because

  • so far in my tests the simple filters work perfectly
  • only the regex filter won't work for me - do you have an example?

Beautiful work, (IBM) Champ!! 😍

@sebjulliand
Copy link
Collaborator Author

@chrjorgensen it runs better now! I fixed what needed to be and RegExp filtering is fine now 😄

Copy link
Contributor

👋 A new build is available for this PR based on 673e6fb.

Copy link
Collaborator

@chrjorgensen chrjorgensen left a comment

Choose a reason for hiding this comment

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

@sebjulliand Regexp works much better now! 😍

One issue was found during testing: the regular express meta characters like \d (for digits) doesn't work, because the filter is changed to uppercase and the \d becomes \D - the reverse!

Can this be fixed? Maybe by not converting regex filter values to uppercase and filter with ignore-case flag? Only if it's easy - I could use [0-9] to achieve the same result. We could also check for any meta characters and send an error message, saying they are not allowed?

Finally only a comment about this implementation and the one in THAT PRODUCT:

  • In THAT PRODUCT the filter can have multiple values, where each value selects generic library AND generic object AND generic member. The multiple values are OR'ed. This is great for a project, where the filter can select e.g. library ABC*, all source files, member DEF* combined with library QWE*, all sourcefiles, member RTY*.

  • In our implementation we have one value, selecting generic library AND generic object AND generic member. This does not allow filtering only some members in library A and other members in library B.

To solve this, we would have to allow a list of filter values (libraries, objects, objects types, member names and member types) and have the output be the combined selection of the filters in the list. How easy would that be? Not easy, I would guess - and this is NOT a challenge! 😉

Copy link
Contributor

👋 A new build is available for this PR based on 4145606.

@sebjulliand sebjulliand removed the build Build will be available inside PR. label Jan 25, 2024
@codefori codefori deleted a comment from github-actions bot Jan 25, 2024
@sebjulliand
Copy link
Collaborator Author

@chrjorgensen I see your point. THAT PRODUCT let's you define filter strings, for members, and OR them.
In our case, we define one filter string per filter.

I guess we could achieve the same result by associating multiple filters, as you said. We would need to list all the members from each filter and display them all at once (I just checked and THAT PRODUCT display them all at the same level).

We would need a different kind of filter; an association of filters, as you said. This should be done in a separate PR if we feel like it's needed. This one is already big enough 😄

@chrjorgensen
Copy link
Collaborator

@sebjulliand

This should be done in a separate PR if we feel like it's needed. This one is already big enough 😄

I agree completely - I was just anticipating the reaction from our users who also have experience with THAT PRODUCT... 😆

@chrjorgensen
Copy link
Collaborator

@sebjulliand I'm happy with this PR - you raise the bar once again! 😍

🚀 🚀 🚀 🚀 🚀 🚀 - 6 out of 6! 😉 😆

@worksofliam @Wright4i Please give this great PR a spin, so we can get it merged.

Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Code is looking good, but please clarify with me so I understand the usage of runSQL - see my comment in the review here for that.

Please also add the test cases back in! (I found that so funny because I do the exact thing). Please request my review when that is done!

image

I am going to be using this for a few hours today, so let me sit on it for a bit. I will make another review.

Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Code looks good, my user testing has been good, and the test cases are passing.

Thank you @sebjulliand!

@sebjulliand sebjulliand merged commit d241dac into master Jan 29, 2024
1 check passed
@sebjulliand sebjulliand deleted the feature/objectFilter branch January 29, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance Object Browser filter's Object field
4 participants