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

Shouldn't extend Base functions on Base types #21

Closed
tkelman opened this issue Aug 16, 2017 · 2 comments
Closed

Shouldn't extend Base functions on Base types #21

tkelman opened this issue Aug 16, 2017 · 2 comments

Comments

@tkelman
Copy link

tkelman commented Aug 16, 2017

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
Copy link
Member

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

@xiuliren
Copy link
Member

xiuliren commented Sep 5, 2017

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

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

No branches or pull requests

2 participants