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

Use real regex instead of strings #174

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

Conversation

lishaduck
Copy link

Fixes #165

Doesn't break tests, which seems good. Haven't tested a custom config yet.

@lishaduck lishaduck mentioned this pull request Jul 10, 2024
@lishaduck lishaduck force-pushed the lishaduck/issue165 branch 3 times, most recently from cdc9ddd to e98c77a Compare July 10, 2024 17:41
@lishaduck
Copy link
Author

Ok, I discovered the examples folder.
The patch seems to work well.

@lydell
Copy link
Owner

lydell commented Jul 11, 2024

Cool! We have this:

module.exports = {
  meta: {
    type: "layout",
    fixable: "code",
    schema: [
      {
        type: "object",
        properties: {
          groups: {
            type: "array",
            items: {
              type: "array",
              items: {
                type: "string", // ????
              },
            },
          },
        },
        additionalProperties: false,
      },
    ],
  },

Do you know how this stuff works? We’re not passing strings now like the schema says, we’re passing RegExp instances.

Other things that I’m noting that needs to be done (either by me or someone else):

  • Probably have tests/examples with both regex and strings.
  • Update docs, including mentioning something about .json config maybe?
  • Maybe validate that the user does not use the g flag, I think that’ll break things?

@lishaduck
Copy link
Author

unicorn doesn't specify it, so I didn't. There's probably a way, but I'm not a JSON schema expert.

@lishaduck
Copy link
Author

Ok, so I just discovered that you can pass a regex into the new Regex constructor, and it'll overwrite the flags. That prevents the "I forgot the u flag" pitfall and the "I added the g flag pitfall", but prevents adding the i flag, and might be too much magic?

@lydell
Copy link
Owner

lydell commented Jul 17, 2024

I think the u flag doesn’t matter to many people, but thanks to the power of copy-paste I think most will use it anyway. So no need to force it in, I would say.

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.

Explore configuration with real regex instead of strings
2 participants