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

Translate comments into English #46

Merged
merged 6 commits into from
Apr 24, 2017
Merged

Translate comments into English #46

merged 6 commits into from
Apr 24, 2017

Conversation

stanislav-tkach
Copy link
Contributor

No description provided.

Copy link
Contributor

@dj8yfo dj8yfo left a comment

Choose a reason for hiding this comment

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

/// 0 | Values, stored in (ind, 0)
/// 1 | Hashes of values from (ind, 0)
///

/// Height is determined by amount of values: `H = len / 2 +1`
/// H | Values
/// 0 | Value hashes
/// 1..| Hashes tree, where each height calculates Hash(Hash(h - 1, i), Hash(h - 1, i + 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

/// Merkle tree over list. Data in table is stored in rows.
/// Height is determined by amount of values: H = log2(values_length) + 1
/// 0 | Values, stored in the structure by index. A datum is stored at (0, index).
/// 1 | Hash of value datum, stored at level 0. (1, index) = Hash((0, index))
/// H > 1 | Merkle tree node, where at position (h, i) = Hash((h - 1, 2i) + (h - 1, 2i + 1)).

+ operation is concatenation of byte arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you test this comment with rustdoc? Maybe more suitable to use markdown lists or tables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried to make a table, but without a header row it looks ugly. I suppose a list also does not fit here, because we have custom "list numbers" here instead of standard numbers/bullets. So I'm just going to add new lines in order to make this look a little better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe add a header row?

Height Stored data
0 Values stored in the structure by index
1 Hash of value datum stored at level 0: (1, index) = Hash((0, index))
> 1 Merkle tree node, where at position (h, i) = Hash((h - 1, 2i) + (h - 1, 2i + 1))

Raw:

| Height | Stored data |
|--------:|--------------|
| 0 | Values stored in the structure by index |
| 1 | Hash of value datum stored at level 0: `(1, index) = Hash((0, index))` |
| > 1 | Merkle tree node, where at position `(h, i) = Hash((h - 1, 2i) + (h - 1, 2i + 1))` |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@slowli slowli left a comment

Choose a reason for hiding this comment

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

I thought there would be more comments in Russian (and comments in general :).

@@ -24,7 +24,7 @@ pub const REQUEST_PREVOTES_MESSAGE_ID: u16 = 8;
pub const REQUEST_PEERS_MESSAGE_ID: u16 = 9;
pub const REQUEST_BLOCK_MESSAGE_ID: u16 = 10;

// когда присоединяются узлы
// Node connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Methinks a period . is unnecessary for such short comments

@@ -37,7 +37,7 @@ message! {
}
}

// консенсус
// Consensus.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these comments supposed to describe message types? They are not very informative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose these comments mean only "this message is part of consensus" as opposed to other "service" messages such as Connect.

Anyway, I see no reason to have comments like "Peers request" at all:

// Peers request.
message! {
    RequestPeers {

Copy link
Contributor

Choose a reason for hiding this comment

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

I had in mind something like

// Request connected peers from the node `to`.
//
// ### Processing
//   * The message is authenticated by the pubkey `from`. 
//     It must be in the receiver's full node list
//   * If the message is properly authorized, the node responds with...
//
// ### Generation
// A node generates `RequestPeers` under such and such conditions...
message! {
     RequestPeers {

Maybe, this is the wrong place to describe messages, but I feel that they should be described somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be great to have such detailed (and meaningful) comments, although I'm solving one specific problem here. Currently we cannot format our code automatically because of comments in Russian. I suppose there should be a separate task for documenting our code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, this is a subject of another PR. Can I open the corresponding issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, feel free to open any issues you think appropriate.

@defuz defuz merged commit 2b7de4a into exonum:master Apr 24, 2017
@stanislav-tkach stanislav-tkach deleted the 44-translate-comments branch July 17, 2017 11:07
stanislav-tkach pushed a commit to stanislav-tkach/exonum that referenced this pull request Feb 10, 2018
stanislav-tkach pushed a commit to stanislav-tkach/exonum that referenced this pull request Feb 17, 2018
stanislav-tkach pushed a commit to stanislav-tkach/exonum that referenced this pull request Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants