pkg / profile

Simple profiling for Go
BSD 2-Clause "Simplified" License
2k stars 122 forks source link

Switched to switch #3

Closed gbbr closed 10 years ago

gbbr commented 10 years ago

I was looking over this with the hope to learn something new and I found it was a great example of using the functional options you spoke about during your talk!

I thought I'd make this change because I personally find it more readable to use a switch when there are multiple if's, it kind of prepares the reader for a lot of conditions. The fact that Go also allows us to have a "no parameter" switch gives us the opportunity to do such things.

This is obviously just a personal opinion and you don't have to merge it if you don't agree.

gbbr commented 10 years ago

I'm not actually sure anymore that this is right. It might be the case that sometimes more than one of those options is on. I just counted on the tests to show me whether things were ok.

Feel free to close it if this last statement is true. Here is palm tree :palm_tree:

davecheney commented 10 years ago

Please reconsider this. I think your PR has value.

Previously, although discouraged, it was possible to enable multiple profiles. But now this is explicitly prevented so use a switch is correct, and we can ensure that one case will always be enabled. On 24 Oct 2014 08:44, "Gabriel Aszalos" notifications@github.com wrote:

Closed #3 https://github.com/pkg/profile/pull/3.

— Reply to this email directly or view it on GitHub https://github.com/pkg/profile/pull/3#event-182999831.

gbbr commented 10 years ago

Ok, well I am reopening it in that case :+1:

Thanks!

sourcegraphbot commented 10 years ago

 func profile.Start(options ...func(*profile.config)) interface{Stop()}


View the full smart diff on ✱ sourcegraph.com

Settings

gbbr commented 10 years ago

I've done the changes. I've also corrected a path in the test since you have now move the package to github.com/pkg - hope that's ok

davecheney commented 10 years ago

LGTM. Thanks.