This repository has been archived by the owner on Jun 25, 2021. It is now read-only.
AD indices refactor #126
Closed
AD indices refactor #126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use unambiguous and consistent name referencing of indices. REFERENCE: maidsafe#120
mrcnski
suggested changes
Nov 15, 2019
src/append_only_data.rs
Outdated
| @@ -581,7 +596,7 @@ macro_rules! impl_appendable_data { | |||
| self.inner.permissions.get(index) | |||
| } | |||
|
|
|||
| /// Returns the owner's public key and the indices at the time it was added. | |||
| /// Returns the owner's public key and the indices at the time it was added. // <--- confusing docs | |||
There was a problem hiding this comment.
I'm not sure if you meant to commit this, but won't this comment appear in the public documentation for this function? I would say either move it into a separate TODO: comment or just address it now.
There was a problem hiding this comment.
I did not actually, I'm sorry it slipped through. It was a note to myself to get to the bottom with what it meant. It can be removed.
mrcnski
previously approved these changes
Nov 16, 2019
src/append_only_data.rs
Outdated
| if owner.entries_index != self.entries_index() { | ||
| return Err(Error::InvalidSuccessor(self.entries_index())); | ||
| /// If the specified `expected_index` does not equal | ||
| /// the count of owners index, an error will be returned. |
There was a problem hiding this comment.
Suggested change
| /// the count of owners index, an error will be returned. | |
| /// the count of owners, an error will be returned. |
Do you think we should change this as well?
There was a problem hiding this comment.
Absolutely, I'll do the change.
mrcnski
approved these changes
Nov 25, 2019
|
Closing this since this data type is being replaced with |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use unambiguous and consistent name referencing of indices.
Closes: #120
This introduces an
AppendOnlyDataindices naming convention, as to avoid confusion and simplify in all repos and on all levels. Applies toEntries,OwnersandPermissions.The goal is to never mix-up what current, next, last index means or refers to, because it does lead to bugs and issues (like off by one error).
Index naming convention
Current-> the index where the last item in the collection is. Can beNonewhen empty.-->
Current itemresides atcurrent index.Expected-> the index that is expected to be current when adding an item, the index where additions are expected.--> A new item to be appended, will then reside at what is now the
expected index. After it is appended, it is thecurrent itemat thecurrent index.Not yet found use case for
PreviousandNext, but they are part of the convention in the following way:From any index we refer to preceding and subsequent index as
PreviousandNextrespectively.From index 0,
PreviousisNone. FromCurrentindex,NextisNone.