Skip to content

Fix bugged `(condp contains? coll ...)` pattern. #18

Closed
wants to merge 1 commit into from

2 participants

@llasram
llasram commented Mar 18, 2013

The pattern (condp contains? coll ...) results in test-expressions like (contains? key coll), which are unlikely to ever be true. This patch replaces these instances with a utility contained? predicated.

@davidsantiago
Owner

Good catch, but I don't like this fix. I think it'd be preferable to just switch these to cond ... (contains? opts :use-existing) (new Row ...) etc. Introducing a new, confusingly named function to support a backwards usage of condp doesn't seem worth it to me. Also, unit tests to make sure these options work as intended would be great to make sure they continue to work as intended.

@llasram
@davidsantiago
Owner

I have other code that has been bitten by this change to contains?; I probably haven't thought through all of the issues, but I continue to have no problem with a dynamically typed language simply answering false to the question of whether a non-container contains something.

It's up to you whether or not to add tests, but I will only take the patch with tests. I'm not really up for doing this work right now either, but I will step into action if someone contributes. I just ask that someone who is giving code for me to maintain do a bit of extra work to make maintenance easier in the future. After all, I never know if I'm ever going to hear from them again (past experience: probably not), and it makes things a lot easier if we can at least try to make sure the same mistakes don't get made over and over. And the fact that this has gone undiscovered for so long is why this could really use better test coverage; the features that are commonly used get a lot of exercise naturally.

Thanks,
David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.