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

Upstreaming the ecto-related parts #113

Closed
kevinlang opened this issue Mar 17, 2021 · 22 comments
Closed

Upstreaming the ecto-related parts #113

kevinlang opened this issue Mar 17, 2021 · 22 comments
Milestone

Comments

@kevinlang
Copy link
Member

I was going to start a new conversation in the elixir-ecto Google group to see what the path forward is for upstreaming most of the ecto related stuff into ecto_sql, now that nearly all of the integration tests are onboarded and stable.

In that scenario, this exqlite repo would mainly contain the core non-ecto driver parts, like the db_connection stuff and the NIF.

Does that sound good, @warmwaffles ? Any reservations or thoughts?

@warmwaffles
Copy link
Member

That's totally okay with me. The only bit that would be staying here would be the db_connection implementation.

@kevinlang
Copy link
Member Author

Cool, here is the thread:

https://groups.google.com/g/elixir-ecto/c/Y27uRzCHSRE

@kevinlang kevinlang reopened this Mar 17, 2021
@lawik
Copy link

lawik commented Mar 17, 2021

I saw that. I hope that isn't too discouraging. I don't think it is really needed to be in ecto_sql to be a good option. Unless I'm missing something.

@kevinlang
Copy link
Member Author

Despite this, I still think it is a good idea to split out the ecto adapter from the actual exqlite driver as we approach 1.0, call it something like ecto_sqlite3. It would be nice for the two to be organized under the same Github organization, perhaps we could merge into the elixir-sqlite org. 🤷‍♂️

@warmwaffles
Copy link
Member

So I was thinking about pulling the adapter into exqlite_ecto. Possibly keep them both in this repository for now similar to Nx.

@warmwaffles
Copy link
Member

What are your thoughts on keeping them both in the same repo but under different directories?

@kevinlang
Copy link
Member Author

I think Nx uses a single repo similar for the way we do it: for ease of rapid iteration until they hit stable. From the README:

they will be extracted to their own repository in the future. 

But I think having them as separate projects in the same repo is fine for now. I do think there is always a tradeoff between keeping things well-separated and maintenance/developer ergonomics, and keeping them in the same repo but be separate Mix projects seems to be a nice middle ground.

@warmwaffles
Copy link
Member

When do you think we should split the adapter? My plan is to open another repo, push this repo into it so we keep the history and then delete the parts that don't belong.

@kevinlang
Copy link
Member Author

kevinlang commented Mar 17, 2021

That sounds like a good plan. I think we can do it whenever.

For naming, some things to consider:

  • https://hex.pm/docs/publish - seems to recommend that the package name should be ecto_{whatever} since it is an ecto-related package.
  • Right now the adapter is Ecto.Adapters.Exqlite, but perhaps should be called Ecto.Adapters.SQLite3, as it is an adapter for the SQLite3 database that only happens to use the :exqlite driver. This is similar to how there is Ecto.Adapters.Postgres which uses the :postgrex driver. I think the main reason why the :myxql adapter is called MyXQL instead of MySQL is because there was a previous MySQL driver that already took that namespace, which MyXQL superseded. I also think it helps with discoverability.
  • Given the two above, I think it makes sense for the repo and package to be called ecto_sqlite3.

What do you think?

@warmwaffles
Copy link
Member

I think I am okay with that plan. We should probably start renaming the adapter now and preparing for that.

@warmwaffles
Copy link
Member

The biggest problem is, people are using this adapter now and it will break their projects if they aren't notified / we don't get a proper warning system in place.

@axelson
Copy link

axelson commented Mar 17, 2021

The biggest problem is, people are using this adapter now and it will break their projects if they aren't notified / we don't get a proper warning system in place.

I wouldn't think you need to worry about that too much. I doubt there's many people who have integrated exqlite since it is such a new project and well known to be under heavy development, so breaking changes at this stage should be expected. In my opinion, a simple note in the new changelog (#114) would be sufficient.

@warmwaffles warmwaffles added this to the 1.0 milestone Mar 17, 2021
@kevinlang
Copy link
Member Author

I started looking into setting up a ecto_sqlite3 repo.

Any reservations around creating an Organization or merging our efforts into the elixir-sqlite org? In either case, I created a simple logo.

logo

@warmwaffles
Copy link
Member

Yea that's fine, we just need access to that org https://github.com/elixir-sqlite/

@kevinlang
Copy link
Member Author

@ConnorRigby, what say you?

@warmwaffles
Copy link
Member

@kevinlang can you shoot me an email at warmwaffles@gmail.com need to co-ordinate how to get you package push permissions and what not.

@kevinlang
Copy link
Member Author

I created an account there kevinlang, which you should be able to use to add me via mix hex.owner add

Here is the repo of ecto_sqlite3 (for now, until the Org is configured):
https://github.com/kevinlang/ecto_sqlite3

And here is the Hex package:
https://hex.pm/packages/ecto_sqlite3

I added you @warmwaffles as another owner.

@warmwaffles
Copy link
Member

@kevinlang okay I'm going to open a PR here to add a changelog and remove the adapter from this repo.

@warmwaffles
Copy link
Member

Also @kevinlang https://twitter.com/pressy4pie/status/1370406636066467840 @ConnorRigby will get us added to that org tomorrow.

@kevinlang
Copy link
Member Author

I transfered the ecto_sqlite3 repo over.

https://github.com/elixir-sqlite/ecto_sqlite3

@warmwaffles
Copy link
Member

Just did the same here too

@kevinlang
Copy link
Member Author

kevinlang commented Mar 18, 2021

Cool. @ConnorRigby do you mind updating the Org logo to what I linked above? I can't since I don't have Owner permissions.

I'll close this one out for now.

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

No branches or pull requests

4 participants