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

fromISO too loose / fromFormat restrictive #1628

Open
angelaki opened this issue May 15, 2024 · 15 comments
Open

fromISO too loose / fromFormat restrictive #1628

angelaki opened this issue May 15, 2024 · 15 comments

Comments

@angelaki
Copy link

I'm just wondering why fromISO accepts pretty much every input while fromFormat need to match 100%:

//Why is this getting parsed?
const iso = DateTime.fromISO('11');
console.log(iso.toISO());
console.log(iso.isValid);

//Why is this *not* getting parsed?!
const input = DateTime.fromFormat('11', 'D');
console.log(input.toISO());
console.log(input.isValid);

(https://stackblitz.com/edit/vitejs-vite-3v4qvn)

I'm asking because (using Angular) I switched to Luxon and their implementation of the Luxon DateAdapter feels pretty broken: https://github.com/angular/components/blob/main/src/material-luxon-adapter/adapter/luxon-date-adapter.ts

But in the end it goes down to that question: why is fromISO so loose / fromFormat so restrictive? Is there an equivalent to old moments' strict: false? To would I need to loop over several input formats (from strict to loose) until the first one matches?

date-fns solved it quite handy: a default date can be passed to fill the missing positions.

@diesieben07
Copy link
Collaborator

The problem with fromISO parsing here is that it allows parsing an ISO time (e.g. 15:07:12) without the T prefix. As such '11' is getting parsed as T11:00:00, which then resolves to "Today at 11am". As far as I can tell, the ISO 8601 spec says that the T should be required, but earlier versions of the spec apparently allowed it.

fromFormat('11', 'D') produces an error, because D means "localized numeric date". "localized" here means "whatever toLocaleString would produce".
'D' maps to { year: 'numeric', month: 'numeric', day: 'numeric' } for toLocaleString, which usually produces something like MM/DD/YYYY or DD.MM.YYYY. '11' does not match that format, as such you get a parse error.

The reason Luxon does not allow loose parsing is because it is error prone and buggy. What is 11/04/2015? Is that April 11th or November 4th? It's impossible to know without knowing the format.

@angelaki
Copy link
Author

angelaki commented May 16, 2024

The problem with fromISO parsing here is that it allows parsing an ISO time (e.g. 15:07:12) without the T prefix. As such '11' is getting parsed as T11:00:00, which then resolves to "Today at 11am". As far as I can tell, the ISO 8601 spec says that the T should be required, but earlier versions of the spec apparently allowed it.

fromFormat('11', 'D') produces an error, because D means "localized numeric date". "localized" here means "whatever toLocaleString would produce". 'D' maps to { year: 'numeric', month: 'numeric', day: 'numeric' } for toLocaleString, which usually produces something like MM/DD/YYYY or DD.MM.YYYY. '11' does not match that format, as such you get a parse error.

The reason Luxon does not allow loose parsing is because it is error prone and buggy. What is 11/04/2015? Is that April 11th or November 4th? It's impossible to know without knowing the format.

Thank you pretty much for this well-founded answer! And from this library's perspective I totally get the decision! But can you maybe give me a recommendation how to solve the following use-case: I want a (localized) user-input to be loosely parsed.

So if a German user enters 16 I'd like to parse this input to 2024-05-16 (current year & month, 16th day). If a US user does, it should fail. But if he enters 05/16 it should be parsed to the same.

Does Luxon offer a method to get the D format for a specific (browser) locale? So I could just reduce it sign by sign, until I get a valid date. And what do you think about date-fns's parse method (https://date-fns.org/v3.6.0/docs/parse)? By passing a referenceDate, the missing information gets added.

Maybe Luxon could support this, too? But with no referenceDate keeping the strict behavior and with a referenceDate included trying a loose parsing (the way I mentioned, by reducing a sign with every loop)? If interessted, I could offer a PR :)

@diesieben07
Copy link
Collaborator

Does Luxon offer a method to get the D format for a specific (browser) locale?

Yes: DateTime.expandFormat. Assuming en-US DateTime.expandFormat('D') would be something like M/d/yyyy. Note that this format is not provided by Luxon, instead Luxon will use Intl.DateTimeFormat#toLocaleParts with a configuration based on the macro token ('D' in this case) and then reverse-engineer the resulting output.

And what do you think about date-fns's parse method (https://date-fns.org/v3.6.0/docs/parse)? By passing a referenceDate, the missing information gets added.

That should be very easy to add. Ultimately all parsing methods end up in DateTime.fromObject, which uses "now" to fill in the gaps. It should be quite easy to just add a parameter here.

You should already be able to achieve this now with this (admittedly quite awful) hack:

const referenceDate = DateTime.whatever(...);
const previousNow = Settings.now;
let parsed;
try {
  Settings.now = () => referenceDate;
  parsed = DateTime.fromFormat(...); // or other parsing methods
} finally {
  Settings.now = previousNow;
}

I am not sure how I feel about the "loose" parsing, because it sounds quite error prone and could lead to parsing stuff the wrong way.

@angelaki
Copy link
Author

angelaki commented May 17, 2024

If everything goes down to fromObject, perfect! Here you go, what do you think about this PR: #1632?

I know this library is pretty much about size so you want to keep it as clean as possible. But guess that'd be a good improvement. Next I'd be thinking about supporting number input in friendlyDateTime - what do you think? So the referenceDate might be a number, too.

And last but not least ... I'd come back to my initial issue: don't you think the lib might offer a strict parameter for fromFormat? Keeping it true by default would keep the library strict as it is! But having this option just would be great! Maybe it is some kind of German thinking: since we always have a format DD.MM.YYYY we really expect inputs to auto-complete the year (& month) for us. If we put in a day only, we expect it to be the day of the current month and year. If we add a month, we expect the year to be autocompleted. We only insert a year, if the context expects it (dates in the past).

Sure this could be achieved by the solution consuming your lib. But wouldn't this make totally sense to be part of Luxon (like it was of moment)?

See this practical example:

Here you see the parse method of Angulars Date Input Control: https://github.com/angular/components/blob/c40cb8003d6ad41178e5290e4c2bc86ab47e832d/src/material-moment-adapter/adapter/moment-date-adapter.ts#L184. The _createMoment method passes an explicit true / false depending on the user's settings. And if you search the net, you might find everyone sets strict to false.

Now check the Luxon's parse method: https://github.com/angular/components/blob/c40cb8003d6ad41178e5290e4c2bc86ab47e832d/src/material-luxon-adapter/adapter/luxon-date-adapter.ts#L174. First of all: why do they parse an ISO before they check the provided formats?! The hell, I don't know. It drives peoples nuts! (Check the web ... It's because of my initial post [2 digit numerics often get parsed wrong]). Next they try to parse the exact format. Users don't type exact formats! Never! The web is full of desperate developers settings strict to false hoping it to work. But there is no strict mode in Luxon (and the Angular Team obviously didn't decide to implement it on their own in their adapter). Sure this logic could be added in the adapter. But doesn't it make so much sence ... quite everywhere? And (imho) the solution is so easy: take the resolved format based on the provided token and reduce it, until you get a valid date. I'd love to see this in Luxon. And the fact that (even) the Angular Team provides a pretty messy adapter shows, that even expirienced developers don't get how to use it.

@angelaki
Copy link
Author

Could really use a loose paring now! What du you guys think? Push this request a bit?

@diesieben07
Copy link
Collaborator

Next I'd be thinking about supporting number input in friendlyDateTime - what do you think? So the referenceDate might be a number, too.

What does this number represent? Seconds since the epoch? Milliseconds? Nanoseconds? It is not clear and therefor I do not think this is a good idea.

First of all: why do they parse an ISO before they check the provided formats?! The hell, I don't know. It drives peoples nuts!

I do not know. This library is not part of Luxon. You'll have to report this issue there.

I still do not understand 100% how you want this "loose" parsing to work. You can't just remove tokens from the format until you get some result out and expect it to somehow be sensible. Just for example:
The basic "date" format in en-US is M/d/yyyyy, but in de-DE it is d.M.yyyyy. How does your loose parsing decide which tokens to ignore from this to try and parse it? If the user enters "23", is that because they meant 2023? The 23rd of the current month? If you can propose an algorithm for how this would work in a generic way that would still produce useful results we can consider how and if to implement it in Luxon.

@angelaki
Copy link
Author

angelaki commented Jun 5, 2024

Just seen that you've also checked my PR for the refDate - thank you pretty much! 👍

Concerning the loose parse-method: I'd expect it to take the resolved token (i.e. M/d/yyyyy in an English environment passing D) and parse the date until it has a valid result:

10/10 : M/d/yyyy --> false
10/10 : M/d/yyy --> false (invalid token)
10/10: M/d/yy --> false
10/10: M/d/y --> false
10/10: M/d/ --> false
10/10: M/d --> true (10/10/2024) [2024 could even better be replaced with the refDate I created a PR for 😊]

Sure it could step on invalid tokens or tokens expecting a alphanumeric input. But it's pretty unlikely they could fit the input. And hey, it is loose! So don't expect it to be perfect. It just tries to guess human input. Guess that's fair.

Imho this is pretty easy implemented and a true added value! But if you don't like it, let's get my PR on the road so I could at least implement a custom parser using this logic.

@diesieben07
Copy link
Collaborator

Personally, I don't think this is something that should be in Luxon, because this is based on your specific needs. Someone else might have other loose parsing needs. In my opinion something like this should live in application code.
@icambron Please let me know if you have a different stance here.

@angelaki
Copy link
Author

angelaki commented Jun 5, 2024

See your point but still the logic is pretty small and easily described. And I guess any other loose parsing would be use-case specific, so no need for other build-in ones.

But yes, sure it could totally even be part of application code! No hard feeling here. I'm just into fromObject supporting a referenceDate 😉

@angelaki
Copy link
Author

angelaki commented Jun 5, 2024

@diesieben07 now that I'm implementing my own parser, I'm noticing a behavior I didn't expect this way (and that makes my loose-parse-suggestion worthless, anyway):

I'm trying to parse a pretty common input 1.1.20 to be 01.01.2020. So I tried the token d.M.y expecting it to sensible parse the year input 20 to 2020. But it sure doesn't. There is a year 20 so sure it needs to expect me to set the year to 20.

Is there a helper-method or similar in Luxon for this? Guess even the refDate wouldn't do it, since there is a year set? date-fns uses the closest input to the refDate so it would become 2020. But is there something similar in Luxon? Since this is a pretty date related use-case I guess Luxon should support here?

Or is it actually the application's job to parse the input first, format it as expected and than use Luxon's parse method? So it actually behaves more like a correct formated string to date rather than a string to date (like I expected it to be)?

@diesieben07
Copy link
Collaborator

y just takes the year literally. yy accepts two digit years:

DateTime.fromFormat('20', 'yy').toISODate(); // 2020-01-01
DateTime.fromFormat('62', 'yy').toISODate(); // 1962-01-01

The "cutoff year" is 1960 currently, but can be configured using Settings.twoDigitCutoffYear.

@angelaki
Copy link
Author

angelaki commented Jun 5, 2024

y just takes the year literally. yy accepts two digit years:

DateTime.fromFormat('20', 'yy').toISODate(); // 2020-01-01
DateTime.fromFormat('62', 'yy').toISODate(); // 1962-01-01

The "cutoff year" is 1960 currently, but can be configured using Settings.twoDigitCutoffYear.

Thank you pretty much, that'll do the job :) Guess there actually is no use-case to build-in support one digit year to behave the same. So this case would be incrementing with leading zero in app-logic I guess?

@diesieben07
Copy link
Collaborator

I have never seen or heard of anyone entering the year "2005" as "5". If you really need to support this then yes, you'll have to prepare the input manually.

@angelaki
Copy link
Author

angelaki commented Jun 5, 2024

Don't get me wrong, but I don't like the design of the Settings.twoDigitCutoffYear being a static variable. This sure can confuse modular applications. How about adding all the needed options always as an override to the opts? I could prepare a PR if I find some time these days.

@diesieben07
Copy link
Collaborator

Sure, a PR that adds a twoDigitCutoffYear option to fromFormat (and related methods) sounds good.

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