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

ref(store): Remove unused RPC Client #672

Merged
merged 2 commits into from
Jan 4, 2023
Merged

ref(store): Remove unused RPC Client #672

merged 2 commits into from
Jan 4, 2023

Conversation

flub
Copy link
Contributor

@flub flub commented Jan 4, 2023

The store does not use the RPC client at all. Remove it.

yolo, let's see what breaks

CI doens't break! It must be good. Teach me why not :)

The store does not use the RPC client at all.  Remove it.
@flub flub requested review from ramfox and dignifiedquire January 4, 2023 12:01
@Arqu
Copy link
Collaborator

Arqu commented Jan 4, 2023

The store does not use the RPC client at all. Remove it.

yolo, let's see what breaks

CI doens't break! It must be good. Teach me why not :)

Is this used on the CLI to get status or whatever from the service? Ie the outside API?
Maybe I just have no clue what I'm talking about though...

@flub
Copy link
Contributor Author

flub commented Jan 4, 2023

The store does not use the RPC client at all. Remove it.

yolo, let's see what breaks

CI doens't break! It must be good. Teach me why not :)

Is this used on the CLI to get status or whatever from the service? Ie the outside API? Maybe I just have no clue what I'm talking about though...

I don't think so, that's done by the rpc server which is still started.

@dignifiedquire
Copy link
Contributor

It was done only in preparation, as the store will need this eventually, to make sure that the architecture is not built such that it assumes the store doesn't need the rpc client.

@flub
Copy link
Contributor Author

flub commented Jan 4, 2023

It was done only in preparation, as the store will need this eventually, to make sure that the architecture is not built such that it assumes the store doesn't need the rpc client.

So can I convince people this doesn't buy us anything? We have plenty of examples to build the RpcClient. A client that's unused in a struct doesn't make us do anything we shouldn't: it is Send and Sync anyway and since it's unused it does not get passed around anywhere. Once there's a use for it I don't think there's going to be any major hurdle to add it.

Personally I find it difficult to reason about things when the types don't match what's unused. The type system should help show what goes where.

@dignifiedquire
Copy link
Contributor

So can I convince people this doesn't buy us anything?

you can, fine to remove for now imo

@flub flub requested a review from dignifiedquire January 4, 2023 16:56
@flub flub merged commit 1d30916 into main Jan 4, 2023
@flub flub deleted the flub/store-no-rpc-client branch January 4, 2023 18:04
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.

3 participants