Clojure

clojure.core/bases returns a cons when passed a class and a Java array when passed an interface

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Release 1.2
  • Fix Version/s: Release 1.5
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

clojure.core/bases returns a clojure.lang.Cons when called with a class as parameter, but a Java array ( [Ljava.lang.Class; ) when called with an interface. Both should return values of the same type, which I guess should be a seq.

Showing the problem at the REPL:
user=> (bases java.util.List)
#<Class[] [Ljava.lang.Class;@315b0333>
user=> (bases java.util.ArrayList)
(java.util.AbstractList java.util.List java.util.RandomAccess
java.lang.Cloneable java.io.Serializable)

I have attached a patch which calls the seq function on the else part of clojure.core/bases. Also updated the clojure.test-clojure.java-interop/test-bases test. However these test do not currently seem to run as part of the maven build. I have however run the test manually and verified that it works.

Activity

Hide
Andy Fingerhut added a comment -

Behavior where bases returns Java array in some cases still exists on latest master, and this patch still applies cleanly and changes that behavior as described.

The patch writer Alf is not on the list of people who have signed a CA. If he isn't in the process of doing so, what should be done with the patch? If I described the behavior to a committer without showing them the patch, they would likely come up with a similar one-line fix.

Show
Andy Fingerhut added a comment - Behavior where bases returns Java array in some cases still exists on latest master, and this patch still applies cleanly and changes that behavior as described. The patch writer Alf is not on the list of people who have signed a CA. If he isn't in the process of doing so, what should be done with the patch? If I described the behavior to a committer without showing them the patch, they would likely come up with a similar one-line fix.
Hide
Alf Kristian Støyle added a comment -

I was not aware that contributors had to sign the CA to get a patch in when I wrote this, so my apologies for adding the patch. I just wanted to make the contribution an "easy add". I guess you guys would have figured out a similar fix really easy.

I am not in the process of signing the CA. I would not have any problems signing it, but then I guess for such a small contribution it would be too much of a hassle. And besides, I don't think this small contribution makes me a worthy contributor.

I would certainly not mind you guys fixing this in anyway you can, with or without my patch.

Cheers,
Alf

Show
Alf Kristian Støyle added a comment - I was not aware that contributors had to sign the CA to get a patch in when I wrote this, so my apologies for adding the patch. I just wanted to make the contribution an "easy add". I guess you guys would have figured out a similar fix really easy. I am not in the process of signing the CA. I would not have any problems signing it, but then I guess for such a small contribution it would be too much of a hassle. And besides, I don't think this small contribution makes me a worthy contributor. I would certainly not mind you guys fixing this in anyway you can, with or without my patch. Cheers, Alf
Hide
Andy Fingerhut added a comment -

No apologies necessary for trying to improve the code, Alf. We appreciate it.

I agree it might be a bit of a hassle to sign a CA just for this fix. Legally, a Clojure contributor is one whether anyone considers them "worthy" or not.

We'll figure something out.

Show
Andy Fingerhut added a comment - No apologies necessary for trying to improve the code, Alf. We appreciate it. I agree it might be a bit of a hassle to sign a CA just for this fix. Legally, a Clojure contributor is one whether anyone considers them "worthy" or not. We'll figure something out.
Hide
Steve Miner added a comment -

Independently written patch that fixes bases so that it always returns a seq. Added additional tests to verify that it returns nil (rather than an empty seq) when there are no bases to maintain compatibility with the original code.

Show
Steve Miner added a comment - Independently written patch that fixes bases so that it always returns a seq. Added additional tests to verify that it returns nil (rather than an empty seq) when there are no bases to maintain compatibility with the original code.
Hide
Fogus added a comment -

Patch 0001-bases-should-return-a-seq-not-a-Java-array.patch from Steve Miner works as expected and is of acceptable quality.

Show
Fogus added a comment - Patch 0001-bases-should-return-a-seq-not-a-Java-array.patch from Steve Miner works as expected and is of acceptable quality.
Hide
Steve Miner added a comment -

I think commit 73f94cb8c7f60a131759b4488a3fefd6599d0328 took the wrong patch. Stoyle said he did not have a CA. I wrote an independent patch which Fogus vetted.

Show
Steve Miner added a comment - I think commit 73f94cb8c7f60a131759b4488a3fefd6599d0328 took the wrong patch. Stoyle said he did not have a CA. I wrote an independent patch which Fogus vetted.
Hide
Stuart Halloway added a comment -

Reverted wrong patch, applied correct patch. Thanks for watching my back, Steve.

Show
Stuart Halloway added a comment - Reverted wrong patch, applied correct patch. Thanks for watching my back, Steve.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: