feat: default value for logos chat client#151
Conversation
jazzz
left a comment
There was a problem hiding this comment.
My previous concerns still remain.
- Blocker: Changing ChatClientBuilder to require an ephemeral base configuration has negative impacts on code maintenance and will be hard to undo in the future.
- This change does not seem necessary at this time, as it does not add any required functionality while removing beneficial properties.
- The motivation of removing a few lines of code doesn't justify the increased maintenance costs in developer applications.
- Forcing Ephemeral be the root of the builder pathway introduces fragmentation, which will become developer pain.
- The focus should be on creating an API that we want in the future( rather that what is available now), and working towards it with little to no breaking code changes in demos and developer applications.
| @@ -0,0 +1,61 @@ | |||
| //! The opinionated Logos client. | |||
| //! | |||
There was a problem hiding this comment.
Having LogosChatClient located within the "Generic client" seems like it will be full of friction as the objects have separate concerns.
the "GenericClient will tend to be more generic/abstract" and the LogosChatClient will tend towards importing specific implementations and services.
[Sand] LogosChatClient would benefit from being a separate crate, to keep imports separated. That way it can import LogosCore specific implementations and GenericClient could remain free of these implementation details
There was a problem hiding this comment.
Split LogosChatClient to its own crate looks too early, we can do it later.
The landscape may also change as we move to production ready, and move ephemeral/in memory dependencies to test.
616df84 to
e2c635b
Compare
Adds an opinionated
LogosChatClientpreconfigured with the Logos stack.Changes:
LogosChatClientwithopen(transport, db_path, db_key, registry_url).db_path/db_keyare caller-supplied;registry_urloverrides the preconfigured endpoint (placeholder until deployed).LogosChatClient::open, so it always uses the registry;--registry-urlis now an endpoint override.Removed the dead
run_logos_delivery.Fix #126