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

Fractional #19

Closed
dimovpetar opened this issue May 31, 2023 · 3 comments
Closed

Fractional #19

dimovpetar opened this issue May 31, 2023 · 3 comments

Comments

@dimovpetar
Copy link

According to Wikipedia, only the smallest unit can be a decimal fraction:

The smallest value used may also have a decimal fraction

Quote from the document that Wikipedia refers to:

b) If necessary for a particular application, the lowest order components may have a decimal fraction.
The decimal fraction shall be divided from the integer part by the decimal sign specified in
ISO 31-0, i.e. the comma [,] or full stop [.]. Of these, the comma is the preferred sign. The decimal
fraction shall at least have one digit, the maximum number of digits in the decimal component
needs to be agreed by the partners in information interchange. If the magnitude of the number is
less than unity, the decimal sign shall be preceded by a zero (see ISO 31-0).

tinyduration lib allows multiple components to have decimal fractions at the same time. I would expect that values like "P0.5YT0.5M" and "P0.5YT5M" are not allowed
https://github.com/MelleB/tinyduration/blob/9d5e55f085326ec7fb5a67188855cdab0477114b/src/index.test.ts#LL16C26-L16C26

@MelleB
Copy link
Owner

MelleB commented Jun 16, 2023

Hi @dimovpetar,

Thanks for bringing this to my attention. This is indeed a mistake on my part, this test-case should not pass.

I've given it some thought as how to fix this and the most feasible option I see is to add a configuration option where you can define if you want to be able to handle this in a strict manner or not. The main reason is that this lib is used quite a bit and I want to avoid introducing breakage in downstream packages by introducing a new runtime exception.

Practically I'd like to propose the following:

  1. Release a minor version where we introduce a config: { strict: boolean } parameter to the parse() method which defaults to false. This includes:
    • Extending the types of the parse function with a new config parameter
    • Adding extra test cases where the functionality is tested
    • Modifying the docs, also making note the fact that the default will change to strict in the next major release.
  2. Release a major version where the strict mode defaults to true - since this is clearly described in the specs.

Would you be willing to create a PR for the first step?

@dimovpetar
Copy link
Author

Hello @MelleB , unfortunately I don't have the time to contribute :)

@MelleB
Copy link
Owner

MelleB commented Jun 29, 2023

@dimovpetar This issue was fixed in #22 . Feel free to open a new issue if you run into any issues! Thanks!

@MelleB MelleB closed this as completed Jun 29, 2023
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

No branches or pull requests

2 participants