[WIP] start rewriting to match server::starter 0.33#11
[WIP] start rewriting to match server::starter 0.33#11
Conversation
lestrrat
changed the title
| "time" | ||
|
|
||
| "github.com/lestrrat/go-server-starter/internal/env" | ||
| tcputil "github.com/lestrrat/go-tcputil" |
There was a problem hiding this comment.
There is no need to alias.
"github.com/lestrrat/go-tcputil"
is equally good.
There was a problem hiding this comment.
That's goimports doing its thing.
lestrrat
force-pushed
the
topic/compat-f10c90f
branch
from
ab4bd4f to
a7eea5f
Compare
lestrrat
changed the title
| func NewLoader(environ ...string) *Loader { | ||
| if len(environ) == 0 { | ||
| environ = os.Environ() | ||
| } |
There was a problem hiding this comment.
Why don't we remove the arguments and change the test because it seems the only intended use case is without them?
| return environ | ||
| } | ||
|
|
||
| func (l *Loader) Iterator(ctx context.Context, options ...Option) *Iterator { |
There was a problem hiding this comment.
What's the point of this iterator? Simply creating a slice of the environment variables would make the code much easier to follow.
|
|
||
| func (l *Loader) Apply(octx context.Context, e Environment, options ...Option) error { | ||
| ctx, cancel := context.WithCancel(octx) | ||
| defer cancel() |
There was a problem hiding this comment.
Canceling after the iterator is finished looks meaningless.
| Value() interface{} | ||
| } | ||
|
|
||
| const LoadEnvdirKey = "LoadEnvdirKey" |
There was a problem hiding this comment.
If all we want is the LoadEnvdirKey flag, then having the interface and the struct feels too much of generalization. What do you think?
| // | ||
| // users of this function can "wait" for the exit of a command | ||
| // by checking the done channel | ||
| func monitor(ctx context.Context, src chan *exec.Cmd, done chan *exec.Cmd) { |
There was a problem hiding this comment.
That was an omission lestrrat/go-server-starter@bb58920
| case chosen == len(workers)-1: | ||
| workers = workers[:chosen] | ||
| default: | ||
| workers = append(workers[:chosen], workers[chosen+1:]...) |
There was a problem hiding this comment.
The default works for all cases.
There was a problem hiding this comment.
Oh wait, I got what you meant. Apologies.
| // users of this function can "wait" for the exit of a command | ||
| // by checking the done channel | ||
| func monitor(ctx context.Context, src chan *exec.Cmd, done chan *exec.Cmd) { | ||
| defer close(done) |
There was a problem hiding this comment.
Why do you need this? This never occurs since the for loop never ends.
There was a problem hiding this comment.
Yes we do. lestrrat/go-server-starter@bb58920
| func findInOptionList(t *testing.T, opts *options, name string, val interface{}) error { | ||
| for _, o := range makeOptionList(opts) { | ||
| switch o.Name() { | ||
| case name: |
There was a problem hiding this comment.
And when we add more options, we rewrite this? no.
| } | ||
| for i := 1; i <= 2; i++ { | ||
| args := make([]string, len(paths)) | ||
| for i, p := range paths { |
There was a problem hiding this comment.
Better to avoid using i. It would be an infinite loop if it was for i, p = range paths.
There was a problem hiding this comment.
No, the outer i controls how many arguments ParseArgs receives. I suppose there are better ways to write this, especially by removing i, but you completely miss the point of the code, and you are just looking at style.
| } | ||
| for i := 1; i <= 2; i++ { | ||
| args := make([]string, len(ports)) | ||
| for i, p := range ports { |
There was a problem hiding this comment.
No, the outer i controls how many arguments ParseArgs receives. I suppose there are better ways to write this, especially by removing i, but you completely miss the point of the code, and you are just looking at style.
|
@edvakf Please ask about the context before making micro-style reviews. There are legitimate points in your review, true. But there's always a history behind code. If this were coming from people whom I have worked with before I'd take this differently, but I don't know you, and you don't know the history behind it. I really don't feel good about random people that I don't know making random style comments without being asked to do so, or trying to understand me and the project history. On the other hand, if you found bugs, please add tests. They are most appreciated. |
|
Apologies. My goal is to get this PR merged. Please ignore comments irrelevant to that direction. |
|
@edvakf it would help immensely if you could try it out, and see if the features you care about are actually working, and report back with results! |
|
日本語で失礼します… ソースコードを読んでいてp5-server-starterとの差異を3つだけみつけたのでご参考までに。
|
|
おお。これテスト書いたほうがいいよねぇ（書きたくない…） FindProcessはその後でやるかな。というか、その辺りはcontext.Contextを使ったほうがよさそうなので、結構あちこちかわるのでは？と思いました。 |
3 participants
after receiving #9, I looked at the original code for the first time in ages, and to my surprise, the original code had changed quite a bit. so here's me trying to match server::starter
0.320.33fixes #10