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

Allow usage with Ecto betas #18

Closed
wants to merge 2 commits into from
Closed

Conversation

crismali
Copy link

No description provided.

@seanabrahams
Copy link
Member

@crismali Thanks for the PR! Do you happen to be using the GraphQL.Relay.Connection.Ecto module (https://github.com/graphql-elixir/graphql_relay/blob/master/lib/graphql/relay/connection/ecto.ex) with Ecto 2 beta and know it to be working? I haven't tested it against Ecto 2 yet and fear that they may not be compatible.

Hopefully it won't be too difficult to support both Ecto 1.x and 2.x with some pattern matching or a conditional if it turns out they're not compatible. Thanks!

@crismali
Copy link
Author

Looks compatible, I haven't had any issues with it so far.

@crismali
Copy link
Author

Any chance of the ecto dependency changing? Right now Ecto betas can't be used even when not using the ecto integration provided in this package.

@seanabrahams
Copy link
Member

@crismali Thanks for pinging for this again. The most recent commit in this PR is a little too restrictive and removes sqlite_ecto which results in failing tests. I understand why you removed sqlite_ecto since it has a dependency on ecto ~> 1.0 with no support for Ecto 2.0 as of yet.

We originally discussed whether Ecto support should be included in this repo and decided to include it since it would be optional. However this is a good example for why separating it is desirable.

I'll look into removing the sqlite_ecto dependency and instead use postgrex for testing so we can get a proper ~> 1.1 or ~> 2.0 dependency for ecto. Alternatively, we can separate out Ecto support. I'll look into this now.

@seanabrahams
Copy link
Member

seanabrahams commented Jun 15, 2016

@crismali After investigating what it would take to support both Ecto 1.x and 2.x in this package it's clear that Ecto support should be moved to another package.

I've created a new issue for this here #22

Thanks for bringing this issue up!

cc @joshprice

@crismali
Copy link
Author

A separate package sounds great, thanks!

@crismali crismali closed this Jun 15, 2016
@seanabrahams
Copy link
Member

In the end it turned out it was easier to update this library instead. Grab v0.4!

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

2 participants