lestrrat-go / server-starter

Go port of start_server utility (Server::Starter)
MIT License
215 stars 21 forks source link

[WIP] start rewriting to match server::starter 0.33 #11

Open lestrrat opened 7 years ago

lestrrat commented 7 years ago

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.32~ 0.33

fixes #10

lestrrat commented 7 years ago

@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.

edvakf commented 7 years ago

Apologies. My goal is to get this PR merged. Please ignore comments irrelevant to that direction.

lestrrat commented 7 years ago

@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!

harukasan commented 7 years ago

日本語で失礼します… まったく時間がとれておらずもうしわけありませんでした。少し前から、手元で試しているかんじでは問題ないように動いています。

ソースコードを読んでいてp5-server-starterとの差異を3つだけみつけたのでご参考までに。 https://github.com/lestrrat/go-server-starter/compare/topic/compat-f10c90f...harukasan:compat

lestrrat commented 7 years ago

おお。これテスト書いたほうがいいよねぇ(書きたくない…) でもよさそうな感じなので、このブランチにPRくださいー。

FindProcessはその後でやるかな。というか、その辺りはcontext.Contextを使ったほうがよさそうなので、結構あちこちかわるのでは?と思いました。