Fixes #35 and #36, other special cases, and adds 100% test coverage

[vsivsi]

HI, I decided to just do this, fixing #35 and #36. Hope you find it valuable.

I tried to keep things as much as possible in the same style, etc. I did need to generalize the test suite a bit because it was previously hard-coded to use "ALTree/bigfloat" which complicated testing the fork in preparation to open the PR. There were a couple of minor changes to get lint to return no warnings, and added tests for both the issue fixes and to reach 100% coverage. I also added an ErrNaN type to mirror what the math/big library itself provides.

Best, Vaughn

[vsivsi]

Hi, a couple of updates, I decided to just exhaustively address all of the special cases listed for math.Pow(), with the exception of NaN since math/big treats it as an error instead of a special value.

https://golang.org/pkg/math/#Pow

[ALTree]

Hi! And thanks for the PR. Just a heads up that I probably won't be able to review this for a few more days.

My plan is to convert the repo to use go modules, tag a v0.2 with the current status, then tag a v0.3 that removes Sqrt in favour of the implementation I merged into the standard library a few months ago, and maybe your patch too. But I'll need to find some free time to do all this.

A few preliminary observation:

• In general, a few small-ish PRs, each focused on a specific issue, are easier to work with. For example here you fixed Panic in Pow for negative base with integer exponent #35, changed the test-files package, and fixed a few golint issue, all in the same PR. Each of those things should be in a separate PR. Small PR are easier to review, and more importantly I will be able to merge right away things I like, while possibly rejecting changes I don't like much (more on this in the next two points).

• golint is a valuable tool, but the policy in many Go projects is to not require the code to be 100% compliant. For example I don't particularly agree with the warning that suggests to always drop the else when the if block ends with a returns. The suggestion is often correct, but in a few cases I find it cleaner to keep the if { } else { } structure. (I'm guessing you changed the code in exp.go to please golint)

• I used bigfloat_test as the package for test files because this makes it easier to be sure that you're testing the API of the bigfloat package as your users see it. It also forces you to write code that looks like the one the package importers will write (since you don't have access to internal stuff), which gives you a good idea about the user-experience of the package users. This is a common and accepted practise. Making the test internal is something that I believe should only be done when necessary (for example, the test in misc_test.go are internal because they need to test un-exported functions). While I understand this may make a little more annoying to test a fork, I don't think we should give-up on this just for that reason.

[vsivsi]

Sure, all points understood. Upon discovering the issues with Pow I honestly expected I'd need to maintain my own vendored fork of this package. Is was only after-the-fact that I decided to throw up a PR just in case you were still interested in maintaining this. Obviously feel free to cherry pick whatever you find to be useful, or reject the whole thing and do it however you want. I generally err on the side of sharing more code rather than holding it back because it "may not be good enough".

Specific points:

1. Yes, as a package maintainer myself: simple, single topic PRs are ideal. But we don't always get ideal! ;-)

2. I agree with not being pedantic about lint output, especially in my own standalone project code. However, for public packages, I generally assume that zero warnings and 100% coverage are considered desirable, unless the contribution guidelines for a package state otherwise. Since this package has no documented guidelines, my default applied.

3. I understand your position, there are good arguments on both sides of this. It's your package, so you can do what you want! It is definitely annoying to test a fork this way, but more seriously it's pretty dangerous because it's possible to make a badly breaking change to a fork, and not quickly detect it (the whole point of unit tests!), because without realizing it you aren't actually testing the modifications made in the fork! It would be very easy to generate a simple PR that is super buggy, but "all of the tests pass!" Generally speaking, these kinds of "broken by default" situations are best avoided, especially in open-source development.

Anyway, thanks again for your detailed response and this very useful package!

[townhill]

it is great I use it for personal reasons!

…

On Tue, 23 Apr 2019, 19:53 Vaughn Iverson, ***@***.***> wrote: Sure, all points understood. Upon discovering the issues with Pow I honestly expected I'd need to maintain my own vendored fork of this package. Is was only after-the-fact that I decided to throw up a PR just in case you were still interested in maintaining this. Obviously feel free to cherry pick whatever you find to be useful, or reject the whole thing and do it however you want. I generally err on the side of sharing more code rather than holding it back because it "may not be good enough". Specific points: 1. Yes, as a package maintainer myself: simple, single topic PRs are ideal. But we don't always get ideal! ;-) 2. I agree with not being pedantic about lint output, especially in my own standalone project code. However, for public packages, I generally assume that zero warnings and 100% coverage are considered desirable, unless the contribution guidelines for a package state otherwise. Since this package has no documented guidelines, my default applied. 3. I understand your position, there are good arguments on both sides of this. It's your package, so you can do what you want! It is definitely annoying to test a fork this way, but more seriously it's pretty dangerous because it's possible to make a badly breaking change to a fork, and not quickly detect it (the whole point of unit tests!), because without realizing it you aren't actually testing the modifications made in the fork! It would be very easy to generate a simple PR that is super buggy, but "all of the tests pass!" Generally speaking, these kinds of "broken by default" situations are best avoided, especially in open-source development. Anyway, thanks again for your detailed response and this very useful package! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#37 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAGLS4FI3SHWVCDLFJWC23PR5LJNANCNFSM4HGEIC2A> .

[vsivsi]

Closing long dormant PR.