search

edgedb / edgedb-cli

The EdgeDB CLI
https://www.edgedb.com/docs/cli/index
Apache License 2.0
166 stars 23 forks source link

Extract the client #112

Closed CodesInChaos closed 4 years ago

CodesInChaos commented 4 years ago

Please extract the client, so it can be used independently from edgedb-cli. I already checked that simply moving client.rs, reader.rs and server_params.rs to a separate crate works (need to make sequences public and adjust some imports) and can prepare pull requests if you want.

tailhook commented 4 years ago

Yes it was recently refactored to make it easier doing that. But while extracting it is easy, I'm not sure we're ready to provide a stable API. I also think we might want to implement connection pooling and uniform interface for connection and connection pool like redis crate does (i.e. move all functional methods to a trait). So it's more work than looks at the surface.

CodesInChaos commented 4 years ago

I don't think anybody expects a stable API when depending on a 0.1 directly from the git repository. But it is enough to start experimenting with it.

Personally I'd use a single repository for edgedb-protocol, edgedb-derive (currently in edgedb-rust), edgedb-client, edgedb-cli (currently in edgedb-cli) and edgeql-parser, edgeql-rust (currently in edgedb). That'd avoid inter repository dependencies (currently they're even circular) and make development easier.

tailhook commented 4 years ago

I don't think anybody expects a stable API when depending on a 0.1 directly from the git repository. But it is enough to start experimenting with it.

Yes. But still breaking on every occassion is a problem. Especially given you can't use a single connection from two different libraries if they depend on different major version. So if we support bindings we have to keep them relatively stable.

Personally I'd use a single repository for edgedb-protocol, edgedb-derive (currently in edgedb-rust), edgedb-client, edgedb-cli (currently in edgedb-cli) and edgeql-parser, edgeql-rust (currently in edgedb). That'd avoid inter repository dependencies (currently they're even circular) and make development easier.

We started from that. But there are more things than just convenience for rust developer. edgeql-rust is the thing edgedb itself depends on, so it's there to conveniently update edgedb things. And edgedb-cli has different versioning approach and needs builds and release tags and so on, that is convenient to use separate repository for that.

CodesInChaos commented 4 years ago

Yes. But still breaking on every occassion is a problem. Especially given you can't use a single connection from two different libraries if they depend on different major version. So if we support bindings we have to keep them relatively stable.

I think in this early stage having an unversioned 0.1 which can have arbitrary breaking changes between commits is fine. It should have settled down a bit before you publish on crates.io. (Speaking of crates.io, you should probably reserve some relevant crate names, they're first-come-first-serve)

And edgedb-cli has different versioning approach and needs builds and release tags and so on, that is convenient to use separate repository for that.

That makes sense

edgeql-rust is the thing edgedb itself depends on, so it's there to conveniently update edgedb things.

The problem is that edgedb-rust (repo) depends on edgeql-rust (repo) via the edgedb-server crate. So building that workspace needs to check out all of edgedb (repo) which in turn pulls in all of postgres. Since some crates in edgedb (repo) depend on edgedb-rust (repo) as well, you have a circular dependency.

Edgedb-cli also depends on edgeql-rust (repo) and it somehow pulls in another different version of postgres.

Since edgedb (repo) already depends on edgedb-rust (repo) since it needs edgedb-, moving edgeql-rust there shouldn't cause much inconvenience there, while it simplifies the dependency structure for everything else.

tailhook commented 4 years ago

Thanks for a deep dive into our dependencies!

Since some crates in edgedb (repo) depend on edgedb-rust (repo) as well

Yes this was a recent addition, we might need rethink it. This is to create protocol messages by edgedb itself. So generally this requires much less coordination between edgedb-rust and edgedb repos, comparing to the lexer crate (edgeql-parser) which parses the language we're working on in edgedb repo (mostly).

Edgedb-cli also depends on edgeql-rust (repo) and it somehow pulls in another different version of postgres.

I think we could solve 90% of the problem by just removing postgres submodule. @1st1, what do you think?

1st1 commented 4 years ago

I think we could solve 90% of the problem by just removing postgres submodule. @1st1, what do you think?

That's a tough question. In short, I'd prefer us to keep the submodule simply because it makes a positive impression on developers who discover edgedb on github. I know it sounds a bit ridiculous. And I know it's not a real blocker. But I'd really prefer if we could find another way of resolving this.

1st1 commented 4 years ago

cc @elprans

elprans commented 4 years ago

Well, the submodule is actually there so that we can build the appropriate version of Postgres for development setups. Removing it means that we'd need to git clone the Postgres repo manually in setup.py or something like that and that adds unnecessary failure modes by itself.

The problem is that edgedb-rust (repo) depends on edgeql-rust (repo) via the edgedb-server crate.

@tailhook, let's just remove edgedb-server from the edgedb-rust repo.

1st1 commented 4 years ago

Well, the submodule is actually there so that we can build the appropriate version of Postgres for development setups.

Yeah, I meant we could have figured out another way of building Postgres for dev purposes, but there are reasons why we'd prefer not to.

@tailhook, let's just remove edgedb-server from the edgedb-rust repo.

+1

tailhook commented 4 years ago

The problem is that edgedb-rust (repo) depends on edgeql-rust (repo) via the edgedb-server crate.

@tailhook, let's just remove edgedb-server from the edgedb-rust repo.

Removed, but this is just half of the problem. Because edgedb-cli still depends on edgedb and pulls different version of edgedb and postgres when installed by edgedb's setup.py (and edgedb installs CLI on build).

tailhook commented 4 years ago

Removed, but this is just half of the problem. Because edgedb-cli still depends on edgedb and pulls different version of edgedb and postgres when installed by edgedb's setup.py (and edgedb installs CLI on build).

On an afterthought, it isn't even half of the problem, because it only makes it faster for building the whole edgedb-rust workspace. Relying on it as a dependency is still the same (as that doesn't instantiate an edgedb-server crate).

tailhook commented 4 years ago

I've merged edgedb/edgedb-rust#30, now we need to remove client from this repository.

CodesInChaos commented 4 years ago

I did that in https://github.com/edgedb/edgedb-cli/compare/master...CodesInChaos:rust-client but it looks like it has merge conflicts now. You can either cherry pick it, or I'll prepare an updated pull request.

tailhook commented 4 years ago

I did that in master...CodesInChaos:rust-client but it looks like it has merge conflicts now. You can either cherry pick it, or I'll prepare an updated pull request.

Done.