Refactor CLI packages #195
Conversation
|
Closing and reopening to trigger travis ... |
masomel
force-pushed
the
refactor-cli
branch
from
a56f056 to
77e9549
Compare
masomel
force-pushed
the
refactor-cli
branch
3 times, most recently
from
559e558 to
c9ad99b
Compare
masomel
force-pushed
the
refactor-cli
branch
from
c9ad99b to
08786f0
Compare
application/config.go
Outdated
| // any kind of CONIKS application-level executable (e.g. key server, | ||
| // client etc.). | ||
| type AppConfig interface { | ||
| InitConfig(file string) error |
There was a problem hiding this comment.
Could be just Init, or Load. I think the latter is more precise.
There was a problem hiding this comment.
Hmm, I thought of this. There already is a Load function in application/config, so I didn't want to have double-naming to reduce confusion.
application/bots/twitterbot.go
Outdated
| if _, err := toml.DecodeFile(path, &conf); err != nil { | ||
| return nil, fmt.Errorf("Failed to load config: %v", err) | ||
| var conf *TwitterConfig = &TwitterConfig{} | ||
| if err := conf.InitConfig(path); err != nil { |
There was a problem hiding this comment.
Please see https://github.com/coniks-sys/coniks-go/pull/195/files#r154509882. Then this could be
var config *TwitterConfig = &TwitterConfig{}
config.Load(path)
|
@c633 When you get a chance, can you please review? |
| // permissions. | ||
| func (server *ConiksServer) Run(addrs []*Address) { | ||
| server.WaitStopAdd() | ||
| go server.EpochUpdate() |
There was a problem hiding this comment.
I think we could do
server.WaitStopAdd()
go func() {
server.EpochUpdate()
server.WaitStopDone()
}
It looks more idiomatic Go.
That being said, I would love to find another way to refactor these APIs.
There was a problem hiding this comment.
I agree, I'm not 100% happy with these APIs. I would be happier if we didn't have to expose the WaitGroup to the server/auditor at all. If you have an alternative suggestion, please let me know! We might find a better way to express this API when implementing the auditor.
There was a problem hiding this comment.
According to places like https://tour.golang.org/concurrency/1, go server.EpochUpdate() is just as idiomatic in Go. Is there a reason why these goroutines should be in anonymous functions?
|
I love what you did with |
Addresses #194
This PR moves the cli binaries to
/cliand refactors some common functionality used to implement various CONIKS executables or client applications in the/applicationpackage.