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

add register action #56

Open
wants to merge 3 commits into
base: master
from
Open

add register action #56

wants to merge 3 commits into from

Conversation

@juliangruber
Copy link
Member

juliangruber commented Dec 2, 2019

$ hypergraph register
✔ Select content module › My module
✔ Select profile module › Julian Gruber
✔ Julian Gruber (af4718575b0ffe87e00cd0ebbf2392004b8bf895d004ebb2793663c4c470bd2d)
✔ My module (0cd93303ded959ac6757de601d356f81ead02bca722a661710c8f7562f590c81)
@juliangruber juliangruber requested a review from chartgerink Dec 2, 2019
Copy link
Contributor

chartgerink left a comment

Superb :) I checked the code and left a few review (and one random) comments.

I am unsure whether my main comment applies or is for @p2pcommons/sdk-js, so tagging @dpaez here 👇

  • The registered content module Dat key is not versioned --> should be versioned (writing non-versioned keys to contents is a separate function, which isn't in the roadmap atm, which the spec specifies as public draft)

Some other relatively minor points

  • The contents Dat key does not have dat:// but the url property does. I don't really care that much, but might be good to be consistent to prevent confusion?
  • LOL here comes the horrible user: I re-registered content twice (total registration is three times) and got nine registered links. Maybe we should add a check for duplicate registration (version specific duplicates, if the above gets revised)
"contents": [
    "0ac8757f9adaf108ef9719225e2674adf12657640045f37ac4658f78f8ebd43d",
    "0ac8757f9adaf108ef9719225e2674adf12657640045f37ac4658f78f8ebd43d",
    "0ac8757f9adaf108ef9719225e2674adf12657640045f37ac4658f78f8ebd43d",
    "0ac8757f9adaf108ef9719225e2674adf12657640045f37ac4658f78f8ebd43d",
    "0ac8757f9adaf108ef9719225e2674adf12657640045f37ac4658f78f8ebd43d",
    "0ac8757f9adaf108ef9719225e2674adf12657640045f37ac4658f78f8ebd43d",
    "0ac8757f9adaf108ef9719225e2674adf12657640045f37ac4658f78f8ebd43d"
  ]
bin/hypergraph.js Show resolved Hide resolved
bin/hypergraph.js Show resolved Hide resolved
index.js Outdated
])
await p2p.register(contentKey, profileKey)
console.log(
`${kleur.green('')} ${kleur.cyan().bold(profile.rawJSON.name)} (${

This comment has been minimized.

Copy link
@chartgerink

chartgerink Dec 2, 2019

Contributor

Could we maybe make that something like ${kleur.green('✔')} ${kleur.cyan().bold(content.rawJSON.title)} (version X) registered to ${kleur.cyan().bold(profile.rawJSON.name)}? Now the log doesn't state what happened.

This comment has been minimized.

Copy link
@juliangruber

juliangruber Dec 3, 2019

Author Member

good idea! I improved the output, but am going to leave this open since we don't have (version X) yet, right @dpaez?

This comment has been minimized.

Copy link
@dpaez

dpaez Dec 3, 2019

@juliangruber right, lets keep this open a bit

@dpaez

This comment has been minimized.

Copy link

dpaez commented Dec 2, 2019

I think these issues belongs to the sdk.

The registered content module Dat key is not versioned --> should be versioned (writing non-versioned keys to contents is a separate function, which isn't in the roadmap atm, which the spec specifies as public draft)

You are right, I need to verify this. Currently we are not properly checking this condition (registered content key to be versioned)

The contents Dat key does not have dat:// but the url property does. I don't really care that much, but might be good to be consistent to prevent confusion?

Also true.

LOL here comes the horrible user: I re-registered content twice (total registration is three times) and got nine registered links. Maybe we should add a check for duplicate registration (version specific duplicates, if the above gets revised)

Yeah, this might happen, there is room for improvements on the checks we are doing on this side. So we can add start by adding a check for duplicate registration. 👍

@chartgerink would you mind creating 3 separate issues with the above feedback on the sdk repo?

Thanks a lot!

@chartgerink chartgerink added the blocked label Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.