-
Notifications
You must be signed in to change notification settings - Fork 4
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
Factor out start-a-libp2p-daemon! #21
base: master
Are you sure you want to change the base?
Conversation
to avoid current-libp2p-daemon problems
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.
If backwards compatibility is not an issue, I am personally in favor of removing current-libp2p-daemon entirely. I personally have not found much use for it in my own code since start-libp2p-daemon! returns a daemon. As far as the code goes LGTM
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.
I don't like your proposed API changes as is. Or at least, you'll have to justify them.
stop-libp2p-daemon! | ||
stop-the-libp2p-daemon! |
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.
Why is it start-a- and stop-the- ?
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.
If backwards compatibility is a concern,
Current behavior has:
start-libp2p-daemon!
for "the" daemon incurrent-libp2p-daemon
stop-libp2p-daemon!
for "a" daemon, wherecurrent-libp2p-daemon
doesn't matter
A better design would have:
start-libp2p-daemon!
for "a" daemonstop-libp2p-daemon!
for "a" daemonstart-the-libp2p-daemon!
for "the" daemon incurrent-libp2p-daemon
stop-the-libp2p-daemon!
for "the" daemon incurrent-libp2p-daemon
The closest thing I could think of to that while maintaining backwards compatibility has:
start-a-libp2p-daemon!
for "a daemon"stop-libp2p-daemon!
for "a" daemon as dictated by existing codestart-libp2p-daemon!
for "the" daemon as dictated by existing codestop-the-libp2p-daemon!
for "the" daemon incurrent-libp2p-daemon
If backwards compatibility is not a concern, I could make it more consistent with only "the" and no "a", but I won't do that unless @vyzo says that's what he wants.
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.
Okay, I've changed the convention to make it more consistent with only "the" and no "a", and also added some documentation.
@@ -14,6 +14,8 @@ | |||
(def current-libp2p-daemon | |||
(make-parameter #f)) | |||
|
|||
;; Starts a new libp2p-daemon only if there is no existing current-libp2p-daemon, | |||
;; and if so sets the current-libp2p-daemon to the new one. |
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.
Maybe that should be renamed ensure-libp2p-daemon
?
Or, if we want backward-compatibility, a force:
or new:
boolean flag?
@@ -45,5 +58,9 @@ | |||
(delete-file path)) | |||
status)))) | |||
|
|||
(def (stop-the-libp2p-daemon!) | |||
(stop-libp2p-daemon!) | |||
(current-libp2p-daemon #f)) |
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.
Shouldn't stop-libp2p-daemon!
always call (current-libp2p-daemon #f)
? And then this function becomes unnecessary?
Honestly, i am fine either way -- i'll let @fare drive this. |
Okay, I think I'll rename them to fit this convention:
|
convention: names with "the" libp2p-daemon convention for functions that deal with current-libp2p-daemon
start-libp2p-daemon!
for "a" daemonstop-libp2p-daemon!
for "a" daemonstart-the-libp2p-daemon!
for "the" daemon incurrent-libp2p-daemon
stop-the-libp2p-daemon!
for "the" daemon incurrent-libp2p-daemon
See also #20