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

Change criteria bracketing behaviour #181

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Change criteria bracketing behaviour #181

merged 1 commit into from
Dec 19, 2024

Conversation

AndrewTwydell
Copy link
Contributor

@AndrewTwydell AndrewTwydell commented Dec 19, 2024

What It Does

Resolves #180

Adds vagueness to the bracketing behaviour to allow a string that ends in brackets but not starts with to be handled correctly. This does add some responsibility to the caller creating the criteria string, but prevents unwanted behaviour like the below example.

Currently, if you pass a string like NOT (PROGRAM=ABC* OR PROGRAM=BCD*), it will send back (NOT (PROGRAM=ABC* OR PROGRAM=BCD*), whereas we would want a bracket at the end too.

Review Checklist
I certify that I have:

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.76%. Comparing base (d75f17d) to head (b9df585).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
- Coverage   93.81%   93.76%   -0.05%     
==========================================
  Files          75       75              
  Lines         776      770       -6     
  Branches       44       42       -2     
==========================================
- Hits          728      722       -6     
  Misses         48       48              

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

Copy link
Contributor

@enamkhan enamkhan left a comment

Choose a reason for hiding this comment

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

LGTM

@davenice
Copy link

davenice commented Dec 19, 2024

I've tested this by pulling the head commit from main, running npm build and then running the branch from within VS Code. I can recreate the problem.

I then pull the bracketing branch, npm build, and run.

I've tried (successfully)...

  1. In a plex, programs, getting the next 500, various other arbitrary resources.
  2. In a single region expanding each possible child.
  3. In an SMSSJ region, expanding each possible child.
  4. newcopy a program.

Copy link
Member

@Joe-Winchester Joe-Winchester left a comment

Choose a reason for hiding this comment

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

Looks good !

@AndrewTwydell AndrewTwydell added the release-patch Indicates a patch to existing code has been applied label Dec 19, 2024
@AndrewTwydell AndrewTwydell merged commit 3786293 into main Dec 19, 2024
17 checks passed
@AndrewTwydell AndrewTwydell deleted the bracketing branch December 19, 2024 15:07
@github-actions github-actions bot removed the release-patch Indicates a patch to existing code has been applied label Dec 19, 2024
@AndrewTwydell AndrewTwydell added the release-patch Indicates a patch to existing code has been applied label Dec 19, 2024
@github-actions github-actions bot removed the release-patch Indicates a patch to existing code has been applied label Dec 19, 2024
@zFernand0 zFernand0 added the release-patch Indicates a patch to existing code has been applied label Dec 19, 2024
Copy link

Release succeeded for the main branch. 🎉

The following packages have been published:

Powered by Octorelease 🚀

@@ -398,12 +398,12 @@ describe('Utils - enforceParentheses', () => {

it("should add first bracket when end exists", () => {
const output = Utils.enforceParentheses("input with spaces)");
expect(output).toEqual("(input with spaces)");
expect(output).toEqual("(input with spaces))");
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tested this yet, but it doesn't seem like having an extra closing ) will work in some scenarios 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-patch Indicates a patch to existing code has been applied released
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

SDK adds incorrect bracketting to criteria
6 participants