Skip to content

Feat: Add minDigitChars #1111

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

will-stone
Copy link

Hi Fabian 👋 🙂

Long time fan of this project. Thought I'd make a contribution...

Added a check for minimum amount of digits in a string. Useful for password validation. Only thing I haven't done is update the i18n package, are we expected to do that?

Also, I noticed that when running pnpm -F valibot test it creates a tsconfig.vitest-temp.json file so have added a commit to ignore that.

Thanks,

Will.

Copy link

vercel bot commented Mar 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
valibot ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 19, 2025 8:43pm

@fabian-hiller
Copy link
Owner

Thanks for the PR! Great idea! Since this is a very specific action, I may wait for more developers to express interest in such a feature before reviewing and merging it.

@fabian-hiller fabian-hiller self-assigned this Mar 31, 2025
@fabian-hiller fabian-hiller added enhancement New feature or request workaround Workaround fixes problem labels Mar 31, 2025
Copy link

pkg-pr-new bot commented Mar 31, 2025

Open in StackBlitz

npm i https://pkg.pr.new/valibot@1111

commit: d6c5269

@will-stone
Copy link
Author

Thanks @fabian-hiller 🙂 If this was accepted, I was going to propose:

  • maxDigits
  • minLetters
  • maxLetters

I'd also like minSpecialCharacters but not sure yet how to do that, that couldn't lead to possible regex injection.

@will-stone
Copy link
Author

If anyone stumbles across this and is looking for a way to implement this now, you can use this:

const minDigits = (min: number) =>
  v.pipe(
    v.string(),
    v.check(
      (input) => (input.match(/\d/gu) || []).length >= min,
      `Must contain at least ${min} digit${min === 1 ? "" : "s"}.`,
    ),
  )

v.safeParse(minDigits(2), 'I have no digits')

@fabian-hiller
Copy link
Owner

In general, I am very interested in providing actions to validate the number of digits, letters and special characters, but we need to find perfect names for them. Valibot usually provides actions like length, notLength, minLength and maxLength to cover every possible case. So I am not sure if we should stick to the name minDigits since we already have a digits action that does not validate the number of digits. So maybe we should rename it to something like minDigitChars. I recommend brainstorming about the perfect names in a first step. In a second step we will make this PR perfect and merge it, and in a third step we can create PRs for all the other actions.

@will-stone
Copy link
Author

Ah, of course; I saw digits and associated it with bytes and length. As for naming, it does feel like digits is the odd one out here. Perhaps it could be enhanced, to avoid a breaking change, like so?

v.digits('The string contains something other than digits.')
v.digits(8, 'Exactly 8 digits are required.')

@fabian-hiller
Copy link
Owner

I don't recommend this as it would make our API inconsistent. What about minDigitChars, minSpecialChars and minLetterChars?

@will-stone
Copy link
Author

Okay let's go with that 👍 🙂 I'll update this PR when I can a moment. Thanks.

@will-stone will-stone force-pushed the feat-min-digits branch 3 times, most recently from 89108b1 to 5582469 Compare April 19, 2025 20:38
@will-stone will-stone changed the title Feat: Add minDigits Feat: Add minDigitChars Apr 19, 2025
@will-stone
Copy link
Author

Okay, that should be good to go; changed name and rebased onto upstream/main 👍 🙂

@fabian-hiller
Copy link
Owner

fabian-hiller commented Apr 19, 2025

Thank you so much! It looks good at first glance. I will try to review and merge it soon. Are you also interested in adding maxDigitChars, digitChars and notDigitChars?

@fabian-hiller fabian-hiller requested a review from Copilot April 19, 2025 23:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new feature to validate the minimum count of digit characters in a string, improving password validation. Key changes include:

  • Adding a new utility function (_getDigitCount) to count digits in a string.
  • Creating a new minDigitChars action with complete type definitions and overloads.
  • Extending API properties and test coverage to support this new feature.

Reviewed Changes

Copilot reviewed 24 out of 34 changed files in this pull request and generated no comments.

Show a summary per file
File Description
website/src/routes/api/(actions)/minDigitChars/properties.ts Adds API property definitions for the minDigitChars action.
library/src/utils/_getDigitCount/* Implements the _getDigitCount utility along with corresponding tests.
library/src/actions/minDigitChars/minDigitChars.ts Implements the minDigitChars action with overloads and type annotations.
library/src/actions/minDigitChars/.test Provides comprehensive tests and type assertions for the new action.
library/src/actions/index.ts Updates the module exports to include minDigitChars.
Files not reviewed (10)
  • website/src/routes/api/(actions)/minDigitChars/index.mdx: Language not supported
  • website/src/routes/api/(async)/customAsync/index.mdx: Language not supported
  • website/src/routes/api/(async)/fallbackAsync/index.mdx: Language not supported
  • website/src/routes/api/(async)/intersectAsync/index.mdx: Language not supported
  • website/src/routes/api/(async)/lazyAsync/index.mdx: Language not supported
  • website/src/routes/api/(async)/pipeAsync/index.mdx: Language not supported
  • website/src/routes/api/(async)/unionAsync/index.mdx: Language not supported
  • website/src/routes/api/(methods)/config/index.mdx: Language not supported
  • website/src/routes/api/(methods)/fallback/index.mdx: Language not supported
  • website/src/routes/api/(methods)/pipe/index.mdx: Language not supported

@fabian-hiller fabian-hiller added this to the v1.2 milestone Apr 20, 2025
@will-stone
Copy link
Author

will-stone commented Apr 20, 2025

I certainly am interested in adding those.

Are there any plans to automate the docs creation? That’s probably the fiddliest part about contributing to this project 😅 I’ve dabbled with typedoc on my personal utility library, Tings, and it works quite nicely. Your setup is more sophisticated as you’re linking pages, but maybe that could go into a JSDoc directive. It would reduce contributor effort, and your review effort massively.

@fabian-hiller
Copy link
Owner

Unfortunately, our docs creation is currently very manual. But in general it should be possible to automate this process by writing a script to extract all necessary information and automatically generate the required index.mdx and properties.ts files. Just in case you are interested in automating our docs creation, feel free to create another PR with a POC for such a script. I am pretty sure that AI should be able to help write it. Maybe it is somehow possible to extract the necessary type information with a custom TypeScript transformer or plugin or by using tsc somehow. I will be happy to donate $50 to you if we end up implementing your script to automate this workflow, as it would save us all a lot of time.

@muningis
Copy link
Contributor

Automation sounds nice, probably would surely make llms.txt easier too.

I just wonder if it would be possible to have those links retained, which links to related schemas/actions/methods.

@fabian-hiller
Copy link
Owner

I'm not looking for a perfect solution. There will still be a manual part. I am more looking for a CLI script that speeds up the process by generating an initial template with all the info we already know from the source code.

@will-stone
Copy link
Author

Thanks @fabian-hiller I'm not sure if I'm going to get the time unfortunately; my time is quite limited these days with family commitments. But if someone else wants to implement this I highly suggest tying it into some custom JSDoc, e.g.

/**
 * Creates a min digits validation action.
 *
 * @param requirement The minimum digits.
 *
 * @returns A min digits action.
 *
 * @schemas any, array, custom, instance, string, tuple, unknown
 * @methods pipe
 * @utils isOfKind, isOfType
 */
export function minDigitChars<

It should get to the point where contributors do not need to touch the docs at all 👍🤓

@fabian-hiller
Copy link
Owner

That is ok. I am sure someone else will look at it in the long run. For now, I prefer to keep our JSDoc comments simple and don't add extra stuff there. We need a separate JSDoc comment for each TypeScript overload function signature, and adding all that info would force us to duplicate it, which is annoying to maintain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request workaround Workaround fixes problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants