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

Added regex validation #20

Merged
merged 6 commits into from
Jul 17, 2023
Merged

Added regex validation #20

merged 6 commits into from
Jul 17, 2023

Conversation

Bullrich
Copy link
Collaborator

Created method which validates the regex expressions in a configuration.

Closes #16

I want to have a constant error type + a custom error log so I make it return either a passed status or a failed status + error message so it can be logged.

@Bullrich Bullrich requested a review from a team as a code owner July 17, 2023 10:35
@Bullrich Bullrich self-assigned this Jul 17, 2023
Comment on lines +35 to +66
export const validateRegularExpressions = (
config: ConfigurationFile,
logger: ActionLogger,
): [true] | [false, string] => {
/** Regex evaluator */
const isRegexValid = (regex: string): boolean => {
try {
new RegExp(regex);
return true;
} catch (e) {
logger.error(e as Error);
return false;
}
};

for (const rule of config.rules) {
for (const condition of rule.condition.include) {
if (!isRegexValid(condition)) {
return [false, `Include condition '${condition}' is not a valid regex`];
}
}
if (rule.condition.exclude) {
for (const condition of rule.condition.exclude) {
if (!isRegexValid(condition)) {
return [false, `Exclude condition '${condition}' is not a valid regex`];
}
}
}
}

return [true];
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could maybe change this to be the following:

Suggested change
export const validateRegularExpressions = (
config: ConfigurationFile,
logger: ActionLogger,
): [true] | [false, string] => {
/** Regex evaluator */
const isRegexValid = (regex: string): boolean => {
try {
new RegExp(regex);
return true;
} catch (e) {
logger.error(e as Error);
return false;
}
};
for (const rule of config.rules) {
for (const condition of rule.condition.include) {
if (!isRegexValid(condition)) {
return [false, `Include condition '${condition}' is not a valid regex`];
}
}
if (rule.condition.exclude) {
for (const condition of rule.condition.exclude) {
if (!isRegexValid(condition)) {
return [false, `Exclude condition '${condition}' is not a valid regex`];
}
}
}
}
return [true];
};
export const validateRegularExpressions = (
config: ConfigurationFile,
logger: ActionLogger,
): boolean => {
/** Regex evaluator */
const isRegexValid = (regex: string): boolean => {
try {
new RegExp(regex);
return true;
} catch (e) {
logger.error(e as Error);
return false;
}
};
for (const rule of config.rules) {
for (const condition of rule.condition.include) {
if (!isRegexValid(condition)) {
logger.error(`Include condition '${condition}' is not a valid regex`);
return false;
}
}
if (rule.condition.exclude) {
for (const condition of rule.condition.exclude) {
if (!isRegexValid(condition)) {
logger.error(`Exclude condition '${condition}' is not a valid regex`);
return false;
}
}
}
}
return true;
};

what do you think?

src/runner.ts Outdated
const [result, error] = validateRegularExpressions(configFile, this.logger);
if (!result) {
this.logger.error(error);
throw new Error("Regular expression is invalid. Check the logs");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of Regular expression is invalid. Check the logs, do

throw new Error(`Regular expression is invalid: ${error.message}`);

?

We wouldn't expect anything aside from descriptive error there, right? (because, there's nothing going on except compiling the regex).

Or, leave it like this, it's good enough already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it makes sense. I don't need to capture the state, only validate that it works or fail.

It also makes it easier for testing

@Bullrich Bullrich requested review from mutantcornholio and a team July 17, 2023 11:55
@Bullrich Bullrich merged commit 1f41374 into main Jul 17, 2023
4 checks passed
@Bullrich Bullrich deleted the validate-regex branch July 17, 2023 12:27
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.

Validate regex expressions
2 participants