search

sociomantic-tsunami / swarm

Asynchronous client/node framework library
Boost Software License 1.0
14 stars 26 forks source link

Add new turtle test node based directly on NodeBase #353

Open gavin-norman-sociomantic opened 5 years ago

gavin-norman-sociomantic commented 5 years ago

(Currently based on #350. Only the last commit is relevant.)

gavin-norman-sociomantic commented 5 years ago

I believe the separation of turtle node from the actual node implementation is a historical artefact from the days when the fake nodes lived partially in turtle. It seems to be unnecessary now.

gavin-norman-sociomantic commented 5 years ago

https://github.com/sociomantic-tsunami/dlsproto/pull/101 gives a rough example of how this change simplifies the code for a fake node.

mihails-strasuns commented 5 years ago

Hm, this doesn't look like any code I have written and turtle definitely doesn't care so I'd say "whatever fits you best" :)

gavin-norman-sociomantic commented 5 years ago

It was derived from code in turtle that you wrote originally.

mihails-strasuns commented 5 years ago

I pretty sure that it was you who wrote that code in turtle originally :) I copied it to swarm with almost no changes when splitting things.

gavin-norman-sociomantic commented 5 years ago

I pretty sure that it was you who wrote that code in turtle originally :) I copied it to swarm with almost no changes when splitting things.

Could be :) Mainly I'm interested in your opinion on the concept, and whether I'm missing some subtlety of turtle.

mihails-strasuns commented 5 years ago

AFAIR turtle only cares that nodes stop printing messages once unregister is called and everything else is completely up to you.

gavin-norman-sociomantic commented 5 years ago

Adapted with working restart. (Still super rough.)

gavin-norman-sociomantic commented 5 years ago

Tidied up.

gavin-norman-sociomantic commented 5 years ago

D2 fixes.

gavin-norman-sociomantic commented 5 years ago

Still TODO:

nemanja-boric-sociomantic commented 5 years ago

Needs rebase now.

gavin-norman-sociomantic commented 5 years ago

Rebased.

nemanja-boric-sociomantic commented 5 years ago

Hmmmm


./src/swarm/node/model/Node.d(702): Error: class swarm.node.model.Node.TestNode interface function INode.restartListeners isn't implemented
./src/swarm/node/model/Node.d(702): Error: class swarm.node.model.Node.TestNode interface function INode.restartListeners isn't implemented
./src/swarm/node/model/Node.d(702): Error: class swarm.node.model.Node.TestNode interface function INode.restartListeners isn't implemented```
nemanja-boric-sociomantic commented 5 years ago

I also left a review on this PR :-)

gavin-norman-sociomantic commented 5 years ago

Ohh shit. I changed INodeBase, but only adapted the neo node classes that implement that interface :/

gavin-norman-sociomantic commented 5 years ago

Updated.

mihails-strasuns commented 5 years ago

@gavin-norman-sociomantic was there anything left for this PR from your side? It looks fine to me but there is a plenty of neo stuff I have no idea about :)

gavin-norman-sociomantic commented 5 years ago

The PR looks sensible to me, in isolation. I think it was waiting on trying to adapt one of the proto fake nodes, to confirm that it works (and is helpful) in practice.

gavin-norman-sociomantic commented 5 years ago

Ah, this appears to be a test of adapting a *proto to use this: https://github.com/sociomantic-tsunami/dlsproto/pull/101