Conversation
frncmx
requested review from
acud,
holisticode,
janos and
zelig
as code owners
frncmx
force-pushed
the
sim-move-node-methods
branch
from
993c416 to
aa0d914
Compare
Only AddNode() had some valid use cases for AddNodeOption, but yet the params were propagated everywhere.
As I surprised myself it works even with 0 or 1 nodes.
Added no useful functionality as it was just combining 2 simple methods and the function was only used in its own test.
The method was used only its own test.
As the concept and the related functionality was not used anywhere.
As the concept and the related functionality was not used anywhere.
That also makes AddNodeOption type and the AddNode's AddNodeOptions parameter unnecessary.
As we can just call Stop() on Network.
The utility of this function was very low. The method just combined two simple calls into one and was used in only one place. Note: verifyRing() now can be private.
This was the lat mixed responsibility function in node.go, i.e., not just adding but connecting nodes. As the responsibility of connecting nodes belongs to p2p/simulations/connect.go I removed the method. Note: there was just one complex use case for AddNodesAndConnectChain in testSwarmNetwork where we actually added nodes more than once and we still wanted to keep them in chain topology. There I introduced a test helper function.
frncmx
force-pushed
the
sim-move-node-methods
branch
from
b73a7d3 to
fc44737
Compare
| // AddNodesAndConnectChain is a helpper method that combines | ||
| // AddNodes and ConnectNodesChain. The chain will be continued from the last | ||
| // added node, if there is one in simulation using ConnectToLastNode method. | ||
| func (s *Simulation) AddNodesAndConnectChain(count int, opts ...AddNodeOption) (ids []enode.ID, err error) { |
There was a problem hiding this comment.
the AddNodeOption should stay. we need to be able to control the configurations of the nodes that are being initialised
There was a problem hiding this comment.
I've found no use case for that in the actual code. Do we have something in progress where we need that opts argument?
There was a problem hiding this comment.
Oh, I've read that but I did not connect the two. Still, there we say "maybe in the future" and I would say YAGNI on that . If that is not too harsh from me. :)
There was a problem hiding this comment.
The commented code was removed eventually. However, AddNodeOption was brought back for AddNode() and AddNodes().
The deleted methods were all unused. So I don't want to maintain them during refactor.
- typos - unused global variable - unused function parameters - redundant import alias - code style issue: snake case
- typos - redundant import alias
frncmx
changed the title
During my code cleanup efforts I removed the AddNodeOption type and related functionality. As that was not used anywhere. However, during code review I was asked to bring back the functionality. Here I'm doing that; still, I would prefer to follow YAGNI.
zelig
left a comment
zelig
left a comment
There was a problem hiding this comment.
i disagree with this entire pr.
in general i wonder why we spending time on one of the best qualuty code in our code base.
typos and unexports aside i would revert all changes.
|
In its current form me too. As mentioned before I disagree with removing the topology functions |
|
Ok. I'll cherry-pick the inspection fixes. @justelad and I had a little confusion around the concept of pivot, if I remember correctly. That concept is also used nowhere. But we still spent time on it and kinda duplicated the functionality when we moved May I keep the commits removing the concept of pivot at least? Other commits I can let go. |
|
Inspection fixes => |
3 participants
This PR is related to #1059 where we want to move
node.gofromswarm/networktop2p/simulations.This PR does not achieve that yet, but already became quite huge. So let's try to merge till I continue the move.