Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

holochain_net: protocol update - HandleGetDht Data & Meta + renames #875

Merged
merged 18 commits into from
Jan 18, 2019

Conversation

ddd-mtl
Copy link
Contributor

@ddd-mtl ddd-mtl commented Jan 16, 2019

(note: must sync with n3h PR: holochain/n3h#27)

Added HandleGetDhtData and HandleGetDhtMeta to follow SendMessage pattern.
Renamed ProtocolWrapper to JsonProtocol.
Renamed some of its enum variants like TrackApp -> TrackDna

Proposed naming convention:

/// Enum holding all message types that serialize as json 'hc-core <-> P2P network module' protocol.
/// There are 4 categories of messages:
///  - Command: An order from the local node to the p2p module. Local node expects a reponse. Starts with a verb.
///  - Handle-command: An order from the p2p module to the local node. The p2p module expects a response. Start withs 'Handle' followed by a verb.
///  - Result: A response to a Command. Starts with the name of the Command it responds to and ends with 'Result'.
///  - Notification: Notify that something happened. Not expecting any response. Ends with verb in past form, i.e. '-ed'.

Misc

Clearer comments and names in test_bin

@ddd-mtl ddd-mtl requested review from neonphog and lucksus January 16, 2019 23:17
@ddd-mtl ddd-mtl changed the title WIP - Net protocol convention update holochain_net: protocol update - HandleGetDht Data & Meta + renames Jan 16, 2019
@@ -86,7 +86,7 @@ pub fn reduce(
/// that lives in the NetworkState.
pub fn send(
network_state: &mut NetworkState,
protocol_wrapper: ProtocolWrapper,
json_message: JsonProtocol,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes more sense, easier to read

@@ -64,7 +64,7 @@ pub struct MessageData {
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, DefaultJson)]
pub struct TrackAppData {
pub struct TrackDnaData {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! Getting consistent on DNA instead of App is a good thing.

@@ -165,120 +165,134 @@ pub struct DhtMetaData {
pub content: serde_json::Value,
}

/// Enum holding all Message types in the 'hc-core <-> P2P Network Module' protocol.
/// Enum holding all message types that serialize as json in the 'hc-core <-> P2P network module' protocol.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, logical naming!

Copy link
Contributor

@neonphog neonphog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much Approve 🐶

@ddd-mtl ddd-mtl merged commit 34609fa into develop Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants