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

Decimal doesn't support non-integer numbers #822

Closed
andrew-3kb opened this issue Sep 8, 2024 · 2 comments
Closed

Decimal doesn't support non-integer numbers #822

andrew-3kb opened this issue Sep 8, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@andrew-3kb
Copy link
Contributor

The decimal validation action will only pass on integers, and not on decimals that aren't integers.

import * as v from 'valibot';
const Schema = v.pipe(v.string(), v.decimal());
const result = v.safeParse(Schema, "2.2");
console.log(result); // success: false (I think it should be true)

I've seen issue #665 on here where someone raised the same thing, which had a explanation that valibot actually means "base ten integer" instead of decimal. I want to make a case that we should consider changing this for the following reasons:

  • It's really useful to be able to validate that a string is a base ten number including non-integers. We do it quite a lot in our codebase. For example we pass around certain number values (like currencies) as strings to avoid floating point issues, and then want to validate they are actually numbers on the other side.
  • There isn't another word I can think of for the above other than decimal, without having a baseTenNumberIncludingNonIntegers or something else really long.
  • I think most people would understand decimal to include non-integers and be confused that we are using it to mean something else. This is anecdotal but actually most of the time I hear the word decimal used it is referring to non-integers.
  • The documentation for the action links to the wikipedia article as it's only definition, but the wikipedia definition explicitly includes non-integers in it's opening line. This was confusing to me when I read the docs to try and understand why my validation was failing. "The decimal numeral system is the standard system for denoting integer and non-integer numbers."

If we want to keep the behaviour of decimal as it is (and backwards compatibility is a great reason) I think the documentation should be updated to be clear it only will validate base ten integers and remove the link to the wikipedia article. It would be nice to add a baseTenNumberIncludingNonIntegers action in this case, hopefully someone can think of a better name though.

Otherwise I propose that decimal is updated to pass on non-integers, and we add a new validation action for the old behaviour called something like baseTenInteger.

I'm happy to do the work and make a pull request for either if you agree with one of the above.

@fabian-hiller fabian-hiller self-assigned this Sep 8, 2024
@fabian-hiller fabian-hiller added the enhancement New feature or request label Sep 8, 2024
@fabian-hiller
Copy link
Owner

I agree that the current implementation of decimal can be confusing. Maybe we should rename decimal to digits and add a new decimal action that accepts any base ten number (even non-integer).

@andrew-3kb
Copy link
Contributor Author

Was fixed by #823

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

No branches or pull requests

2 participants