add Aqua.jl tests and fix ambiguity
#283
Conversation
|
@LilithHafner can you review this? I guess this repo isn't very active these days. |
| return S′ isa StructArrayStyle ? typeof(S′)(Val{N′}()) : StructArrayStyle{typeof(S′), N′}() | ||
| end | ||
| function BroadcastStyle( | ||
| ::Union{SparseArrays.HigherOrderFns.SparseMatStyle, SparseArrays.HigherOrderFns.SparseVecStyle}, |
There was a problem hiding this comment.
The aqua addition looks good to me. CI is all red here, even when it was green on master Saying something about "SparseArrays" not found. That should probably get fixed. Maybe this line?
I have not reviewed the changes to src/structarray.jl.
There was a problem hiding this comment.
should I import SparseArrays here?
There was a problem hiding this comment.
Seems like a bad idea to add SparseArrays as a dependency just for this. Maybe a weak dependency if necessary?
There was a problem hiding this comment.
There is a SparseArrays extension now, so this may be moved to that?
| ::Union{SparseArrays.HigherOrderFns.SparseMatStyle, SparseArrays.HigherOrderFns.SparseVecStyle}, | ||
| ::StructArrays.StructArrayStyle{S, 0} | ||
| ) | ||
| return Unknown() |
There was a problem hiding this comment.
not sure what's the correct style here, but before it would result in ambiguity error so this isn't regression
There was a problem hiding this comment.
I don't like that. Ambiguity errors exist for a reason. If it is truly ambiguous then somebody who understands what is going on needs to disambiguate it, setting it to Unknown() could be worse than an ambiguity error.
| return S′ isa StructArrayStyle ? typeof(S′)(Val{N′}()) : StructArrayStyle{typeof(S′), N′}() | ||
| end | ||
| function BroadcastStyle( | ||
| ::Union{SparseArrays.HigherOrderFns.SparseMatStyle, SparseArrays.HigherOrderFns.SparseVecStyle}, |
There was a problem hiding this comment.
should I import SparseArrays here?
3 participants
Also doc test seems to be failing on 1.10+ because of the
@NamedTupleprinting