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

Support datetime-local time parsing #603

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

stephendolan
Copy link
Member

@stephendolan stephendolan commented Jan 12, 2021

This PR intends to close #602.

You can read more about DateTime Local inputs at this link:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/datetime-local

From what I can gather, browsers basically use RFC 3339 without the timezone. Because of this, our Time::Format::RFC_3339 parser wasn't actually extracting the time element from a datetime-local input type in Lucky.

PR Components

  • [TimeExtensions#parse] Add a custom Time::Format that will parse these datetime-local inputs correctly, though it will always use Time::Location.local for the timezone.
  • [Specs] Moved the hardcoded strings to use time formatting where possible so that we can test more date/time ranges and check specific time values more easily.
  • [Specs] Add some checks on specific parsed time components so that we can be more confident in the future that we're not missing components of the time.
  • [Specs] Removed a test scenario for space_separated_yaml. Best I can tell, that was added here without any additional details, but it has never actually worked since we didn't have a formatter that matched down to the time element. Without a clear user story, I didn't feel confident adding a custom parser that would allow the new test validations to pass for that format.

Copy link
Member

@matthewmcgarvey matthewmcgarvey left a comment

Choose a reason for hiding this comment

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

Nice work on this 🎉🎉🎉

@stephendolan stephendolan merged commit 84e431c into master Jan 12, 2021
@stephendolan stephendolan deleted the sd-fix-datetime-local-time-parsing branch January 12, 2021 14:18
@jwoertink
Copy link
Member

I have to dig in more later, but this PR breaks specs for me locally:

Failures:

  1) Time column type .parse casts various formats successfully
     Failure/Error: "#{result.value.hour} #{_format}".should eq("#{time.hour} #{_format}")

       Expected: "8 iso8601"
            got: "16 iso8601"

     # spec/type_extensions/time_spec.cr:25

I added some debugging to the spec to see which time format is breaking. I don't want to open an issue just yet since I'm not sure if this is only me, but I'm running these locally on my mac (Catalina 10.15.7), and not in Docker. Also, I'm in PST timezone.

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

Successfully merging this pull request may close these issues.

Lucky datetime input not persisting time element
3 participants