-
-
Notifications
You must be signed in to change notification settings - Fork 658
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
refactor: remove check for feature naming data object #4745
Conversation
This is already handled by the method itself, so we don't need to check it twice (we're not Santa Claus, after all).
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -3,53 +3,43 @@ import { IFeatureNaming } from '../../types/model'; | |||
const compileRegex = (pattern: string) => new RegExp(`^${pattern}$`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played around with some minor refactoring. WDYT?
import { IFeatureNaming } from '../../types/model';
const compileRegex = (pattern: string) => new RegExp(`^${pattern}$`);
const disallowedStrings = [' ', '\\t', '\\s', '\\n', '\\r', '\\f', '\\v'];
// Helper functions for error messages
const whitespaceError = (pattern: string) =>
`Feature flag names cannot contain whitespace. You've provided a feature flag naming pattern that contains a whitespace character: "${pattern}". Remove any whitespace characters from your pattern.`;
const exampleMismatchError = (example: string, pattern: string) =>
`You've provided a feature flag naming example ("${example}") that doesn't match your feature flag naming pattern ("${pattern}").`;
const noPatternError = (type: string) =>
`You've provided a feature flag naming ${type}, but no feature flag naming pattern. You must specify a pattern to use a ${type}.`;
export const checkFeatureNamingData = (featureNaming: IFeatureNaming) => {
const { pattern, example, description } = featureNaming;
const errors: string[] = [];
if (disallowedStrings.some((str) => pattern?.includes(str))) {
errors.push(whitespaceError(pattern as string));
}
if (pattern && example && !compileRegex(pattern).test(example)) {
errors.push(exampleMismatchError(example, pattern));
}
if (!pattern && example) {
errors.push(noPatternError("example"));
}
if (!pattern && description) {
errors.push(noPatternError("description"));
}
return errors.length ? { state: 'invalid', reasons: errors } : { state: 'valid' };
};
export type FeatureNameCheckResult =
| { state: 'valid' }
| {
state: 'invalid';
invalidNames: Set<string>;
};
export const checkFeatureFlagNamesAgainstPattern = (featureNames: string[], pattern?: string): FeatureNameCheckResult => {
if (pattern) {
const mismatchedNames = featureNames.filter((name) => !compileRegex(pattern).test(name));
return mismatchedNames.length ? { state: 'invalid', invalidNames: new Set(mismatchedNames) } : { state: 'valid' };
}
return { state: 'valid' };
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! But it does have one issue: the first two checks in checkFeatureNamingData
are mutually incompatible. But I can touch that up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there's some typescript type issues with the return in the checkFeatureNamingData
function.
Second, I'm not sure I agree about adding the extra ternary return in checkFeatureFlagNamesAgainstPattern
, so I'll leave that off if you don't mind.
Implemented in 05f46f1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the Santa reference! :D
We already check for its presence before doing anything. We don't need to
check it twice (we're not Santa Claus, after all).