Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix broken test #398

Merged
merged 2 commits into from Jun 20, 2013
Merged

fix broken test #398

merged 2 commits into from Jun 20, 2013

Conversation

UnixJunkie
Copy link
Member

I don't have this locally and I don't know how it happened there

gasche added a commit that referenced this pull request Jun 20, 2013
@gasche gasche merged commit 11e5e9a into ocaml-batteries-team:master Jun 20, 2013
@gasche
Copy link
Member

gasche commented Jun 20, 2013

After giving some more thought, I think I was maybe too harsh on the use of dummy_node (). In some cases such as this one, it is indeed rather easy to get rid of them (at the price of a small duplication of logic that you handled well), but in other cases such as for nsplit it would have been rather non-trivial (it works well if it's easy to compute the first element of the result list).

I have been thinking about ways to encapsulate this dummy_node under interfaces that hide the unsafeness. One obvious option is to have a reference to an option (None if no elements were added yet, and Some cell otherwise) and handle that in accum, but that comes at a performance cost that is not acceptable.

What I'm saying is: don't spend too much efforts getting rid of dummy_node (). The unit tests, on the contrary, are very much appreciated (that's the main reason why I merged the combine commit). However, I think that you should maybe try to cover more cases in your tests: we want to have all the edge cases, and here we are missing tests for the exception-raising case. Tests for the "obvious" use of the functions are valuable as documentation and to catch really stupid refactoring mistakes, but regression tests are infinitely helpful to catch changes "on the edge" when the implementation changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants