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
Look for meridiem indicator when matching iso format #2985
Comments
Ouch! That one looks like it might bite someone for sure. No deprecation warning either. |
Seems like maybe just making sure the ISO checker goes all the way to the end of the string suffices, e.g. by adding a |
@icambron I don't think we can do that. Currently the following test passes: assert.ok(moment('2016-01-01 is my date').isValid(), 'test extra chars after iso date') It's almost a certainty that there is someone out there in the world doing that. Change the regex and that test fails (along with a couple others that I haven't hunted down yet). |
Hmm, OK. My take would be to deprecate that and fix this AM/PM thing when we drop support for it. I don't see a good reason why we should support |
I've been thinking about this a bit, and I kind of wonder if the least-invasive answer to some of the parser issues we're seeing - like this one - would be to actually default the parser to strict mode. It seems like more often than not, forgiving parsing issues (like this one) are solved by switching to strict mode (because the user should have been using strict mode in the first place). Maybe this is one of those 'help people fall into the pit of success' things? That would leave existing functionality possible, but push the dev down the right path. |
Agree, we've long wanted to do that. I think it's been listed as a possible 3.0 item for a long time. But certainly having the automagic one-arg signature be strict is a small step towards that. |
I actually started coding strict-mode-by-default today by adding a toggle-able global state variable called globalStrict, which I defaulted to true. The strict mode setting can then be switched back to false by calling: moment.globalStrict(false) This makes everything behave the way it always did, without having to change every call to moment's parser. Alternately, the globalStrict setting could default to false, and we could strongly encourage developers to set it to true in the documentation. This is less invasive and maybe helpful? I could also, as suggested, just default one-argument calls to strict mode. That will upset the probably thousands of people who are still falling back to JS date construction though. |
I like all of that, except one thing:
To be clear, my suggestion wasn't to finish off the can't-fall-back-to-JS-constructor deprecation. In fact, the opposite: in this case, we want to do just that but the ISO check is preempting us. |
So, are you saying you would try the global state thing, or that you would avoid it? Maybe I'll just finish the unit tests and make the PR and we can talk about it there. |
Sorry, I was unclear: I'm not against the global state thing at all. I just don't think it precludes us from making the |
@maggiepint thanks for pointing me to the right thread. So whats the easiest way to turn on strict parsing by default ? (sorry, being lazy) |
@Aukhan we never implemented global strict because of an issue with strict mode parsing that needed to be fixed first. (but it was, so we may yet get there). To accomplish what you're doing, I think you just need to specify the ISO_8601 constant and strict mode inline in your code: moment("2016-04-06Tnull", moment.ISO_8601, true).isValid()
false |
@maggiepint thanks again for your reply ... That's a great suggestion but I wouldn't want to replace hundreds of such expressions where not just the ISO_8601 but different custom formats are being used, so I guess the global strict option would be nice to have. I may not have the required skills but if I can help out in any way please let me know. |
Closing in favor of #3502 |
As presented in #2983, using the date format "2016-02-23 11:31:23 PM" will match an ISO format, even though it is not. This causes a wrong date to be parsed:
This is because 2016-02-23 11:31:23 technically is an ISO format.
We need to change the from-string file to check for a meridiem indicator and do something other than match ISO format if it is there.
The text was updated successfully, but these errors were encountered: