search

kachayev / fn.py

Functional programming in Python: implementation of missing features to enjoy FP
Other
3.35k stars 203 forks source link

Curried bug fix #74

Closed cad106uk closed 9 years ago

cad106uk commented 9 years ago

When I used the curried decorator my system threw an error.

So I have made a test and a bug fix (and some pep8'ing) here.

Hope you like it.

Digenis commented 9 years ago

You don't describe the bug. "Threw an error" doesn't suffice and catching an AttributeError in the tests is not self documented.

Many of your pep8 changes in tests removed space intended for alignment. If you intent to run tools such as autopep8, I find it more constructive of you to open a PR listing the settings with which you configure the tools so that people can discuss on them, rather than scrolling through huge diffs manually. Although I find many sensible ones, can we really have a discussion over hundreds of lines of diff output?

The test does too much, ending up reducing its utility. First catching an exception and then re-raising it with a custom message hiding the original traceback. Despite the usefulness of tests for curried, which was merged in #50 without a single testcase, I find this one inappropriate. You could fit 11 lines in a: self.assertEqual(4, curried(_+_)(1)(3)) and even test the returned value. Let me note that this line is a different test case: usual functions, class and instance methods, lambdas, partials and build-ins should all be tested, but I find the try/except very redundant.

Overall, your PR goes off the track, making different changes from the ones promised in the title: -Applying coding standards in advance to the whole files that you want to work with. -Changing gitignore (with a commit that also does more than it says).

Also, you make the changes in the opposite order: You first fix the bug and then write a test. Going the other way around makes reviews much easier: Write a test that demonstrates the bug and makes the build fail, then fix the bug. (you did first experience the bug as you said, it wasn't a blind shot, so show us what you saw before fixing)

cad106uk commented 9 years ago

Far enough. Later this week (or weekend) I'll take another crack at this. I will close this PR and start a new one with your feed back in mind.

The bug itself is a little different that just running the curried decorator. So I'll have to rethink the tests for this.

I'll refrain from the pep8'ing :)

Thank you for the constructive feedback.