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: support more actions in JSON schema conversion #998

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

Conversation

43081j
Copy link

@43081j 43081j commented Dec 27, 2024

Adds support for the following actions in JSON schema conversion:

  • bic
  • cuid2
  • decimal
  • digits
  • empty

@43081j 43081j force-pushed the actions-everywhere branch from 7d32cf3 to a931fd2 Compare December 27, 2024 17:10
Adds support for the following actions in JSON schema conversion:

- `bic`
- `cuid2`
- `decimal`
- `digits`
- `empty`
@43081j 43081j force-pushed the actions-everywhere branch from a931fd2 to 274c950 Compare December 27, 2024 17:11
@fabian-hiller
Copy link
Owner

I first need to check that the .source of a regex assigned to the .pattern does not produce any unexpected behaviour or bugs. At the moment I suspect that this implementation is buggy because .source does not contain any regex flags and JSON schema does not support regex flags as far as I know. Flags like i for case insensitivity would therefore lead to bugs if missing.

@43081j
Copy link
Author

43081j commented Dec 27, 2024

no worries

you are right that the flags will be lost during conversion. not a lot to be done about that

we could throw/skip if any flags are set, or we could accept that the conversion is lossy

@fabian-hiller
Copy link
Owner

I am not sure how to proceed. So far the only important flag we are using is i for case insensitivity. We could remove this flag by rewriting these regular expressions so that they can simply be passed to .pattern. The drawback would be that they might become a bit more complex to maintain, and I expect a small increase in bundle size. Supporting only the regular expressions without the i flag could be another option, but this is a buggy option as we may introduce bugs when changing a regex.

@43081j
Copy link
Author

43081j commented Dec 27, 2024

I think we do both:

  • throw when flags are set, or don't set the pattern
  • try rewrite some of the simpler patterns to no longer need flags

if it turns out this isn't possible (especially since we use u a lot), maybe we just cant support these actions in conversion?

we don't support any of them yet, so throwing seems to make more sense than "best effort"

@fabian-hiller
Copy link
Owner

fabian-hiller commented Dec 27, 2024

I think we can ignore the u flag for JSON Schema. This flag usually has no effect and we only use it because it seems to be a good practice for newer regular expressions.

I will look into rewriting some or all of the regular expressions. However, our v1 RC release has a higher priority. So it may be a while before we can move forward with this PR.

@andersk
Copy link
Contributor

andersk commented Dec 27, 2024

Perhaps we can translate the i flag using a modifier: /foo/i/(?i:foo)/? This isn’t recommended but is probably better than ignoring it.

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.

3 participants