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

Feature/datetime #7

Merged
merged 7 commits into from Sep 6, 2016
Merged

Feature/datetime #7

merged 7 commits into from Sep 6, 2016

Conversation

crisward
Copy link
Contributor

@crisward crisward commented Sep 2, 2016

Got the read and writing of date/time working.

If I've missed anything, let me know.

@crisward
Copy link
Contributor Author

crisward commented Sep 2, 2016

Think the failing test may be due to the mysql version, I think the microsecond support maybe recent. It's passing locally.

http://stackoverflow.com/questions/2572209/why-doesnt-mysql-support-millisecond-microsecond-precision

travis uses 5.6.? I think - https://docs.travis-ci.com/user/build-environment-updates/2015-04-09/

@crisward crisward closed this Sep 6, 2016
@bcardiff
Copy link
Member

bcardiff commented Sep 6, 2016

Hey @crisward , why this PR was closed?

@crisward
Copy link
Contributor Author

crisward commented Sep 6, 2016

I've included it in the text field PR. I wanted to test them together, I also fixed the failing test (see open PR)

@bcardiff
Copy link
Member

bcardiff commented Sep 6, 2016

Oh, It would be better to do it in separate I think. If you were able to fix the failing test I was going to ask for a squash everything here in a single commit so I can merge it. I will do some comments in the text/blob PR.

MySql/DateTime-Crystal/Time is less related to text/blob than MySql/Time-Crystal/Time::Span.
So actually we can go on and on adding conversions, but let's do it one at a time.

If it's too much bother, I can pick it up from here and get your changes for DateTime support. As you prefer. I am glad for your contributions and I don't to push them back.

@asterite
Copy link
Member

asterite commented Sep 6, 2016

@bcardiff When you decide to merge a pull request you can choose to squash the commits, so there's no need to ask this anymore :-)

@bcardiff
Copy link
Member

bcardiff commented Sep 6, 2016

I forgot about that :-) even so, we need the fix for the test. So @crisward if you can bring that change here I should be able to squash it.

@crisward
Copy link
Contributor Author

crisward commented Sep 6, 2016

ok will do

@crisward crisward reopened this Sep 6, 2016
@bcardiff bcardiff merged commit f540d49 into crystal-lang:master Sep 6, 2016
@crisward crisward deleted the feature/datetime branch September 8, 2016 10:28
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.

None yet

3 participants