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

Add support for parse_strings_as_datetimes to expect_column_values_to… #423

Merged
merged 3 commits into from Apr 15, 2019

Conversation

jcampbell
Copy link
Member

@jcampbell jcampbell commented Apr 12, 2019

Address #422

testing note: I have done a manual test on the sqlalchemy adaptation present here, but unfortunately the sqlite db doesn't handle datetimes well enough to deal with our IN statement required in this case. There may be other idiosyncrasies across different systems as well.

Before merging:
[ ] Review updates to test config to see if it will be possible to either (1) test this using sqlite or (2) migrate testing to a fuller-featured db.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 94.871% when pulling 0d11698 on fix/values_datetimes into 103b962 on develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 94.871% when pulling 0d11698 on fix/values_datetimes into 103b962 on develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 94.871% when pulling 0d11698 on fix/values_datetimes into 103b962 on develop.

@coveralls
Copy link

coveralls commented Apr 12, 2019

Coverage Status

Coverage increased (+0.01%) to 94.965% when pulling 4586e75 on fix/values_datetimes into 103b962 on develop.

@abegong
Copy link
Member

abegong commented Apr 12, 2019

These changes totally make sense.

Do you want to merge this as is, or implement tests against another DB first?

I'd lean towards setting up the DB---travis makes this pretty easy with PostgreSQL, as long as you're hitting a supported version: https://docs.travis-ci.com/user/database-setup/

@jcampbell
Copy link
Member Author

I'd say let's merge these separately...I created a separate branch with its own PR #427 that adds support for testing these against postgres (using travis).

It's good to have the additional detail, because the underlying message is that we're straining the laissez faire GE approach to typing as we are more explicit about additional DB types.

Copy link
Member

@abegong abegong left a comment

Choose a reason for hiding this comment

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

👍

@jcampbell jcampbell merged commit febca46 into develop Apr 15, 2019
@jcampbell jcampbell deleted the fix/values_datetimes branch April 15, 2019 11:31
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