Skip to content
This repository has been archived by the owner on Jan 6, 2020. It is now read-only.

Removes PublicAnMaid: Maid 1053 #48

Merged
merged 6 commits into from
May 25, 2015
Merged

Conversation

mmoadeli
Copy link
Contributor

what is done in the PR
1- Removes PublicAnMaid.rs
2- Adds revocation_public_key as a member of public_maid and public_mpid
3- minor clean up

Please note in order to keep the PRs review-able, the issues regarding proper name creation (which is asked to be done by David) will done in other tasks.

Review on Reviewable

@maidsafe-highfive
Copy link

r? @dirvine

(maidsafe_highfive has picked a reviewer for you, use r? to override)

@mmoadeli mmoadeli changed the title Maid 1053 Removes PublicAnMaid: Maid 1053 May 24, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+1.72%) to 93.59% when pulling 37ff723 on mmoadeli:MAID-1053 into 443382f on maidsafe:master.

@dirvine
Copy link
Contributor

dirvine commented May 24, 2015

Review status: 0 of 5 files reviewed, 1 unresolved discussion, all commit checks successful.


src/id/public_mpid.rs, line 36 [r1] (raw file):
As this is now exactly the same as public_maid with the the exception that it holds a different tag value should we not be considering reducing this code dramatically and have a ID type that pmid maid etc. as well as public couterparts are all derived from. It seems a pretty large duplication of code here and we know what we code we can break, so perhaps consider reducing this to an ID type and use a u64 template parameter to create the specific types we want?


Comments from the review on Reviewable.io

@dirvine
Copy link
Contributor

dirvine commented May 24, 2015

I think we can dramatically reduce code here using an ID and Public Id types with the corresponding revocation types. Lets look a bit closer at that as it will make things much simpler.


Review status: 0 of 5 files reviewed, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@mmoadeli
Copy link
Contributor Author

Agreed, thanks


Review status: 0 of 5 files reviewed, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@dirvine
Copy link
Contributor

dirvine commented May 24, 2015

Cool, It also makes our types extensible as app devs can create their own ID types with just a bit more work by us here. I can see this become an important area.


Review status: 0 of 5 files reviewed, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@coveralls
Copy link

Coverage Status

Coverage increased (+1.89%) to 93.76% when pulling 414f675 on mmoadeli:MAID-1053 into 443382f on maidsafe:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.56%) to 93.43% when pulling a1ee947 on mmoadeli:MAID-1053 into 443382f on maidsafe:master.

@dirvine
Copy link
Contributor

dirvine commented May 25, 2015

Is the intention here to not have an id type that can create maid/mpid etc. now? We still have a ton of duplication!


Review status: 0 of 7 files reviewed, 1 unresolved discussion, some commit checks pending.


Comments from the review on Reviewable.io

@mmoadeli
Copy link
Contributor Author

I did so for Public types only

@dirvine
Copy link
Contributor

dirvine commented May 25, 2015

Ok, lets get the docs 100% and then we can get the code in place to match :-)


Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.
Reviewed files:

  • Cargo.toml @ r2
  • src/id/mod.rs @ r2
  • src/id/public_an_maid.rs @ r1
  • src/id/public_id_type.rs @ r3
  • src/id/public_maid.rs @ r2
  • src/id/public_mpid.rs @ r2
  • src/lib.rs @ r2

Comments from the review on Reviewable.io

dirvine added a commit that referenced this pull request May 25, 2015
Removes PublicAnMaid: Maid 1053
@dirvine dirvine merged commit 44591dd into maidsafe-archive:master May 25, 2015
@mmoadeli mmoadeli deleted the MAID-1053 branch May 27, 2015 15:13
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.

None yet

4 participants