-
Notifications
You must be signed in to change notification settings - Fork 5
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 Peer Discovery Code #40
Conversation
6474f75
to
006d2e9
Compare
006d2e9
to
30056bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general note. This is awesome. But we need to discuss domain modelling details.
Things inside bin should not be imported. Bin things should be completely independent, self-contained programs that each result in an CLI executable.
…On 11 June 2020 12:09:35 GMT+10:00, Robbie Cronin ***@***.***> wrote:
@robert-cronin commented on this pull request.
> import initPolyKey from '../initPolykey'
+import { actionRunner, pkLogger, PKMessageType } from '../polykey'
I got stuck here, I'm not sure how to use the module level imports for
this one, at the moment ***@***.***` resolves to the `src` directory.
Perhaps we could have another `tsconfig path` for the bin folder?
something like ***@***.***` perhaps?
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#40 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
Yea this shouldn't be in bin.
…On 11 June 2020 12:06:53 GMT+10:00, Robbie Cronin ***@***.***> wrote:
@robert-cronin commented on this pull request.
> @@ -1,10 +1,12 @@
import fs from 'fs'
`initPolykey` was more intended to be a function used to get a polykey
instance up and running for each command called by the user. It's not
exposed to the end user. It also prompts the user in case there are any
things missing from polykey's config like private key or passphrase or
location. We could put it in the main `polykey.ts` file perhaps?
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#40 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
d8b891a
to
e7c592c
Compare
1a7eebe
to
b2a844b
Compare
So from our previous meeting I have removed a lot of methods from the Polykey class and put them in their respective classes according to which domain model they belong to. |
b2a844b
to
d38e259
Compare
Can you give a brief description of what was done so I know what was applied on a high level from our discussion. |
So the main changes are:
Note: I think it's probably better if I do WIP commits with good descriptions from now on and leave the squashing to the end for more clarity for the reviewer. |
I'd like to see more integration tests in the near future for this kind of code especially as it interacts with the network. But also all the edgecases, we'll have to plan that out. I'm going to approve for merging so you can start synchronizing the other work. |
Woops I was trying the Github merge options. But I think it just left the WIP commits there. Not a big deal, but need to remember to use the squash option, not rebase option. |
I've rebased and squashed so the master is clean again. |
Delete the above branch if you don't need it again. |
This PR will include a
PeerStore
for keeping track ofPeerInfo
. A multicast broadcasting class for peer discovery on the same LAN and various other bug fixes (e.g. relative@
imports).Edit: this PR now also includes a mechanism for discovering a social user via a handle (
robert-cronin
) and a service (github
) - relates to #41Fixes MatrixAI/TypeScript-Demo-Lib#1