search

seung-lab / BigArrays.jl

Storing and accessing large Julia array locally or in cloud storage without server.
Apache License 2.0
13 stars 3 forks source link

Shouldn't extend Base functions on Base types #21

Closed tkelman closed 7 years ago

tkelman commented 7 years ago

This is known as type piracy and can change the behavior of unrelated code. Particularly your file index.jl defines a lot of Base method extensions on CartesianRange, CartesianIndex, and UnitRange. If those are going to be added anywhere, they should really be added in Base first. If accepted there (if it's agreed that those should be defined), then they could be backported to Compat. Otherwise it would be best to avoid global side effects and instead define package-local types, where you can define whatever behavior you want and it won't change anyone else's code just from importing your package. There are also a few method extensions in h5s.jl that are on functions or types from HDF5 - HDF5.h5read{N}(chunkFileName::AbstractString, H5_DATASET_NAME::AbstractString, rangeInChunk::CartesianRange{CartesianIndex{N}}) and Base.setindex!{T,N}(dataSet::HDF5.HDF5Dataset, buf::Array{T,N}, rangeInChunk::CartesianRange{CartesianIndex{N}}).

If either the function or at least one of the input types is from this package, then there's no problem and no code that isn't related to this package would have its dispatch change.

xiuliren commented 7 years ago

I might create a PR to Julia. Indexing array using CartesianRange might be generally acceptable.

xiuliren commented 7 years ago

@tkelman issue fixed. The bad Base functions were all renamed to avoid polluting the global name space. Just created a new package registration PR: https://github.com/JuliaLang/METADATA.jl/pull/11067