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

fix: 🐛 fixes an incorrect check for a string instead of regexp #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thkruz
Copy link

@thkruz thkruz commented Jan 9, 2024

I previously thought we were looking for strings since we were converting them into regular expressions. It turns out we were passing in regular expressions and should have been typecasting them instead of creating new regular expressions out of the old ones.

I previously thought we were looking for strings since we were converting them into regular expressions. It turns out we were passing in regular expressions and should have been typecasting them instead of creating new regular expressions out of the old ones.
if (typeof this.data !== 'string') {
throw new Error('nameRegex must be a string');
if (typeof this.data === 'string') {
throw new Error('nameRegex must be a RegExp');
Copy link
Owner

Choose a reason for hiding this comment

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

Actually there may be a small issue, since we are now loading the groups from the config.json, since here the regular expressions are strings and we haven't done any conversions to a RegExp. For this reason we should probably support both strings and RegExp, converting any string to RegExp:

    let regex: RexExp; 
    if (typeof this.data === 'string') {
      regex = new RegExp(this.data);
    } else if (this.data instanceof RegExp) {
      regex = this.data;
    } else {
      throw new Error('nameRegex must be a RegExp or a regex in string form');
    }

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.

2 participants