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: add support for new constraint operators #289

Merged
merged 12 commits into from Feb 21, 2022
Merged

Conversation

ivarconr
Copy link
Member

No description provided.

@ivarconr ivarconr marked this pull request as draft January 20, 2022 20:59
@coveralls
Copy link

coveralls commented Jan 20, 2022

Coverage Status

Coverage decreased (-0.7%) to 93.801% when pulling b4858fe on feat/constrain-operators into f709c58 on main.

examples/simple_usage.js Outdated Show resolved Hide resolved
examples/simple_usage.js Outdated Show resolved Hide resolved
examples/simple_usage.js Outdated Show resolved Hide resolved
examples/simple_usage.js Outdated Show resolved Hide resolved
examples/simple_usage.js Outdated Show resolved Hide resolved
examples/simple_usage.js Outdated Show resolved Hide resolved
examples/simple_usage.js Outdated Show resolved Hide resolved
}
default:
return false;
}
Copy link
Member Author

@ivarconr ivarconr Jan 20, 2022

Choose a reason for hiding this comment

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

Instead of a switch on the Operator-enum I am tempted to instead create a small handler function per operator and just execute that function.

Example for contains:

const handler = (values: string[], contextValue) => 
   values.some((val) => contextValue.includes(val.trim()))

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we should probably pre-map the values by trimming them, making the handler work on pure data.

values.map((v) => v.trim())

src/helpers.ts Outdated

const prefix = info
? info.username
: `generated-${Math.round(Math.random() * 1000000)}-${process.pid}`;
Copy link

Choose a reason for hiding this comment

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

opt.semgrep.node_insecure_random_generator: crypto.pseudoRandomBytes()/Math.random() is a cryptographically weak random number generator.
(at-me in a reply with help or ignore)

Copy link
Member Author

Choose a reason for hiding this comment

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

@sonatype-lift ignore

@ivarconr ivarconr marked this pull request as ready for review January 29, 2022 15:17
Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

This looks really good. Adds a lot of value to constraints :)

const DateOperator = (constraint: Constraint, context: Context) => {
const { operator } = constraint;
const value = new Date(constraint.value as string);
const currentTime = context.currentTime ? new Date(context.currentTime) : new Date();
Copy link
Member Author

@ivarconr ivarconr Feb 3, 2022

Choose a reason for hiding this comment

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

we might want to move this to the creation of the context.

(+) The benefit will be that the date and time will not change between multiple constriants within the same feature toggle.
(-) The downside would be that we would have to create the currentTime date instance on the context, even when a use is not using any constraints that uses date operators.

@ivarconr ivarconr merged commit 2c92960 into main Feb 21, 2022
@ivarconr ivarconr deleted the feat/constrain-operators branch February 21, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants