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

feat: matches basic operator #1121

Merged
merged 1 commit into from
Dec 14, 2023
Merged

feat: matches basic operator #1121

merged 1 commit into from
Dec 14, 2023

Conversation

sdo-1A
Copy link
Contributor

@sdo-1A sdo-1A commented Dec 7, 2023

Proposed change

Currently it is not possible to have in the conditions something like the following:

stringFact matches pattern

Where "stringFact" is whatever string and "pattern" is a regular expression like "/^8[0-9]{3}/"

Related issues

@sdo-1A sdo-1A requested a review from a team as a code owner December 7, 2023 16:21
Copy link

nx-cloud bot commented Dec 7, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c4aee84. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

export const matchesPattern: Operator<string, string> = {
name: 'matchesPattern',
evaluator: (value, pattern) => {
const regExp = new RegExp(pattern);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not as simple: new RegExp('/a/i') is not equivalent to /a/i but /\/a\/i/.
You will need to handle the 2 / of the regexp and the options given in the end (that are the second parameter of the new RegExpp())

@kpanot kpanot linked an issue Dec 8, 2023 that may be closed by this pull request
@sdo-1A sdo-1A force-pushed the feat/matches-operator branch from 52bfb7e to 9eb8083 Compare December 8, 2023 15:24
@@ -119,7 +119,11 @@ export const allLower: Operator<number[], number | string> = {
export const allMatch: Operator<string[], string> = {
name: 'allMatch',
evaluator: (array, inputString) => {
const regExp = new RegExp(inputString);
let regExp = new RegExp(inputString);
Copy link
Contributor

@kpanot kpanot Dec 9, 2023

Choose a reason for hiding this comment

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

can be instantiated without be used.

    if (inputString.startWith('/')) {
      const finalSlash = inputString.lastIndexOf('/');
      const regexpContent = inputString.slice(1, finalSlash);
      const regexpOption = inputString.slice(finalSlash + 1);
      return new RegExp(regexpContent, regexpOption);
    }
    return new RegExp(inputString);

@@ -71,6 +72,8 @@ Example of usage :
| All[] | lengthGreaterThanOrEquals | number of ≥ | Check if the number of values of the variable is greater or equal to a specific value |
| All[] | lengthGreaterThan | number of > | Check if the number of values of the variable is greater than a specific value |

> **Note**: For the operators comparing a text variable to a pattern, the special characters used in the patterns should contain a double backslash (`\\`).
> For example, to check if a string contains a `/`, the pattern would need to include `\\/`.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also specify that you support /^myRegExp.*$/i(ES RegExp) and ^myregexp.*$ (regexp content).
(also, with my suggestion, the \\/ is not mandatory, it is already not mandatory in the regexp content format)

@@ -119,7 +119,11 @@ export const allLower: Operator<number[], number | string> = {
export const allMatch: Operator<string[], string> = {
name: 'allMatch',
evaluator: (array, inputString) => {
const regExp = new RegExp(inputString);
let regExp = new RegExp(inputString);
if (inputString[0] === '/') {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you want to match an url, wouldn't it detect it as a regex? Ex: /this/is/a/url.com

Would that be a valid scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point, I will add to the documentation that a parameter of type regexp content that begins with a / should be preceded by \ to avoid a wrong detection

Copy link
Contributor

Choose a reason for hiding this comment

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

while that is correct and would work as a mitigation, I wonder if it does not make sense to apply your logic if the inputString begins and ends with a '/' ?

@sdo-1A sdo-1A force-pushed the feat/matches-operator branch from 9eb8083 to a4d7418 Compare December 11, 2023 17:16
kpanot
kpanot previously approved these changes Dec 12, 2023
const regexp = new RegExp(regexpPattern, regexpFlags);
return array.every((elementValue) => regexp.test(elementValue));
}
return array.every((elementValue) => RegExp(inputString).test(elementValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

Reduce the cost if you instantiate the regExp only once:

Suggested change
return array.every((elementValue) => RegExp(inputString).test(elementValue));
const regExp = new RegExp(inputString);
return array.every((elementValue) => regExp.test(elementValue));

Comment on lines 102 to 107
if (inputString.startsWith('/')) {
const finalSlash = inputString.lastIndexOf('/');
const regexpPattern = inputString.slice(1, finalSlash);
const regexpFlags = inputString.slice(finalSlash + 1);
return RegExp(regexpPattern, regexpFlags).test(value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic seems to be duplicated 3 times, might be good to extract the code in an helper

const inputRegExp = parseRegExp(inputString);

or something

@sdo-1A sdo-1A force-pushed the feat/matches-operator branch from c4aee84 to 4a64581 Compare December 14, 2023 08:19
@sdo-1A sdo-1A added this pull request to the merge queue Dec 14, 2023
Merged via the queue into main with commit d1b1f53 Dec 14, 2023
26 checks passed
@sdo-1A sdo-1A deleted the feat/matches-operator branch December 14, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: "Matches" as a basic operator for strings in rules
5 participants