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

Implement the basic functions of AppendOnlyData #14

Merged
merged 3 commits into from Jun 5, 2019

Conversation

Projects
None yet
4 participants
@nbaksalyar
Copy link
Member

commented Jun 4, 2019

This PR implements some of the AppendOnlyData functions as they are described in the RFC.
Currently, it lacks the permissions handling and response types.

@nbaksalyar nbaksalyar requested review from lionel1704 and Yoga07 Jun 4, 2019

@nbaksalyar nbaksalyar requested a review from ustulation as a code owner Jun 4, 2019

@nbaksalyar nbaksalyar requested a review from m-cat Jun 4, 2019

@lionel1704
Copy link
Member

left a comment

Didn't consider the Request and Response type too much. They would probably change once the mock implementation is started.
The rest looks good 👍 Just a few comments.

@@ -0,0 +1,542 @@
// Copyright 2019 MaidSafe.net limited.
//
// This SAFE Network Software is licensed to you under The General Public License (GPL), version 3.

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Jun 5, 2019

Member

License needs to be changed 😄

fn get(&self, key: &[u8]) -> Option<&Vec<u8>>;

/// Get a list of keys and values with the given indexes.
fn at(&self, start: Index, end: Index) -> Option<&[(Vec<u8>, Vec<u8>)]>;

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Jun 5, 2019

Member

Should we consider changing the name of this function?
at seems like getting an entry at a particular index. Maybe we can consider something like in_range or between or from_range

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jun 5, 2019

Author Member

Agreed, and I also just spotted "indexes" which should be spelled "indices" 😄 Thanks!

fn permissions_index(&self) -> u64;

/// Get a complete list of permissions from the entry in the permissions list at the specified index.
fn permissions(&self, start: Index, end: Index) -> Option<&[P]>;

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Jun 5, 2019

Member

We can perhaps consider including range in this function and in the owners() function too

@@ -9,11 +9,11 @@

use serde::{Deserialize, Serialize};
use std::collections::BTreeMap;
use std::error::Error;
use std::error::Error as StdError;

This comment has been minimized.

Copy link
@m-cat

m-cat Jun 5, 2019

Contributor

Minor style point: I would delete this line and just refer to it as ::std::error::Error, since I personally don't like renaming imports unless they are heavily used.

This comment has been minimized.

Copy link
@Yoga07

Yoga07 Jun 5, 2019

Member

I guess, without renaming, it'd conflict with the enum Error?

This comment has been minimized.

Copy link
@m-cat

m-cat Jun 5, 2019

Contributor

If you refer to it using its entire name (::std::error::Error) it shouldn't conflict with anything.

}

#[derive(Copy, Debug, Hash, Eq, PartialEq, PartialOrd, Ord, Clone, Serialize, Deserialize)]
pub struct AppendOnlyDataRef {

This comment has been minimized.

Copy link
@m-cat

m-cat Jun 5, 2019

Contributor

Looks like this struct is identical to MutableDataRef. Are separate structs necessary here?

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jun 5, 2019

Author Member

I'd be in favour of separating concerns here. They may look identical, but semantically they refer to different things: then, if the AppendOnlyData addressing schema changes (like e.g. we replace type_tag with something else), we wouldn't need to decouple it from MutableDataRef.

nbaksalyar added some commits Jun 5, 2019

feat/append-only-data: address review points
- Use correct licence header
- Use better naming for AppendOnlyData functions (at -> in_range, permissions ->
  permissions_range, owners -> owners_range).
@m-cat

m-cat approved these changes Jun 5, 2019

@nbaksalyar nbaksalyar merged commit ddf7a0e into maidsafe:master Jun 5, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@nbaksalyar nbaksalyar deleted the nbaksalyar:append-only-data branch Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.