propose chain_index::Queryable as the name of the capability #671
Conversation
|
|
||
| use std::sync::Arc; | ||
|
|
||
| impl<T> NonFinalized for Arc<T> |
There was a problem hiding this comment.
I think if we're to remove words from this trait name (again, less information? Why?) then it's more useful to call this thing a 'Snapshot'. The fact that it is of the non-finalized state is useful for understanding the underlying mechanics of the snapshot process, but it's irrelevant to the consumer.
There was a problem hiding this comment.
(whoops. This comment is on the impl block, meant to put it on the NonFinalized trait itself)
There was a problem hiding this comment.
Since this is the snapshot module, and it provides the impl to get a snapshot of the non_finalised_state, it seems to me like this trait should be called something along the lines of Snapshottable or plain Snapshot.
There was a problem hiding this comment.
again, less information? Why?
I think less information in the last part of the name, can stimulate the reader to consider the other parts of the name.. i.e. the mod path. So.. often I think it's better to not replicate a mod name in the items it contains.. this practice can couple nicely with a practice of partial path imports.. so one could:
use chain_index::snapshot;
let x = snapshot::NonFinalizedIn that way the explicit and verbose naming information IS included but with the context that let's the reader immediately know about the definition's local mod hierarchy.
There was a problem hiding this comment.
Discussed on call...The non-finalized state is used by the chain index a lot. If there's a module/submodule relation, than snapshot should be inside non_finalized_state and not the other way around...however, the snapshot being of the non-finalized is useful information for a zaino-developer...but at the public API level this is irrelevant, the snapshot is a component of the chain index. I think the snapshot and non_finalized_state mods should both be in the chain_index mod.
There was a problem hiding this comment.
You prefer snapshot and non_finalized_state to be sibling mods within chain_index?
There was a problem hiding this comment.
Is it right to say that the chain index takes a snapshot of itself?
There was a problem hiding this comment.
I do. Another possibility that I just thought of is to have the snapshot mod be inside the non_finalized_state mod, make sure that it's not public, and then pub use the snapshot mod in chain_index. This means that snapshot is clearly inside non_finalized_state from a developer perspective, but make them siblings at the public API level (as the snapshot being of the nonfinalized state is useful for developers to know, but unnecessary confusion to consumers)
There was a problem hiding this comment.
If this were the organization then the name of the trait might make more sense as "SnapShotable"? Instead of "NonFinalized"?
|
I have long been a fan of long explicit names.. this proposal shifts somewhat from that in favor of encouraging readers to account for the enclosing mod name to contextualize a given item. |
|
I fixed a doc lint, and I think I addressed all of the current comments. I didn't resolve any conversations, as I suppose it might make more sense for the initiator of conversations to click resolve. I'm hoping that @AloeareV and/or @nachog00 are interested in continuing the conversation that this PR is derived from. I'd be curious if @nachog00 had any comments to add here. |
zancas
changed the title
|
|
||
| use std::sync::Arc; | ||
|
|
||
| impl<T> NonFinalized for Arc<T> |
There was a problem hiding this comment.
Since this is the snapshot module, and it provides the impl to get a snapshot of the non_finalised_state, it seems to me like this trait should be called something along the lines of Snapshottable or plain Snapshot.
There was a problem hiding this comment.
Is it right to say that the chain index takes a snapshot of itself?
| /// ```no_run | ||
| /// # async fn example() -> Result<(), Box<dyn std::error::Error>> { | ||
| /// use zaino_state::{ChainIndex, NodeBackedChainIndex, ValidatorConnector, BlockCacheConfig}; | ||
| /// use zaino_state::{Queryable, Index, ValidatorConnector, BlockCacheConfig}; |
There was a problem hiding this comment.
this import should be through the chain_index module path, for clarity
|
@nachog00 can you push this forward? I don't want to fail to capture our collective mental model upgrade, but I can't make this a top-priority in the immediate term. |
|
I think we're all in agreement that snapshot can properly be private inside non_finalised_state, and only have a very specific entrypoint exported as pub? |
|
Here's the structure of the mods after the changes proposed (to this point) in this PR: cargo modules structure -p zaino-state --focus-on zaino_state::chain_index --no-fns --max-depth 1
crate zaino_state
└── mod chain_index: pub
├── struct Index: pub
├── trait Queryable: pub
├── struct Subscriber: pub
├── mod encoding: pub
├── mod finalised_state: pub
├── mod mempool: pub
├── mod non_finalised_state: pub
├── mod snapshot: pub
├── mod source: pub
└── mod types: pubBy contrast see the start state in the tracking issue: |
Fixes: #719
Renaming the ChainIndex to Queryable has a few benefits.
(1) It hints to a reader that the thing is not a type.
(2) It conforms to a notion of a Capability that a thing can be made to offer by implementing it.
(3) It frees the name "ChainIndex" to be used less ambiguously on related types.
(4) It reduces a redundancy in the naming scheme when the mod name is also accounted for.
At least as a thought experiment, @nachog00 reconsidered related trait and type names and scopes and opened this PR as a proposal for an update to the naming scheme.