search

JuliaIO / LibExpat.jl

Julia interface to the Expat XML parser library
Other
9 stars 32 forks source link

Broken on 0.4 for exporting `find` #30

Closed garborg closed 9 years ago

garborg commented 9 years ago

http://pkg.julialang.org/?pkg=LibExpat&ver=nightly

Locally:

INFO: Testing LibExpat
PASSED 1
Warning: both LibExpat and Base export "find"; uses of it in module Main must be qualified
ERROR: LoadError: UndefVarError: find not defined

It doesn't seem like LibExpat should extend Base.find since it doesn't return and index and doesn't always return an array. Is there an alternative name that fits the domain? Or if keeping find is preferable, we could just always qualify it rather than exporting it.

garborg commented 9 years ago

@amitmurthy Any thoughts on which direction you want to go with this?

amitmurthy commented 9 years ago

Isn't it OK for LibExpat's find to extend Base.find? Though the return types are different, the intent is the same and since it works off a LibExpat.ETree there should be no issues.

I thought performance considerations only arose when a given function implementation returned different types as in http://docs.julialang.org/en/latest/manual/performance-tips/#write-type-stable-functions

If you think extending Base.find is an issue, I would prefer to go with the qualifying option, i.e., keep the name find.

garborg commented 9 years ago

Sorry, I think I mixed in irrelevant issues -- wasn't trying to say type instability was the thing. Rather that returning the object searched for (rather than the index, as Base.find does) doesn't fit the pattern for that function, and extending find anyway because the verb fits, that sounds like the "punning" that is getting rooted out of Base, from what attention I've paid to the language's issue comments.

amitmurthy commented 9 years ago

@vtjnash any thoughts on this? You are the other major user of LibExpat that I know.

garborg commented 9 years ago

@amitmurthy I'm sure @vtjnash has a more informed view, but here's why it makes sense to me to follow the the grain here -- apart from punning being misleading to users (both for the specific method, and the injected inconsistency hurting usability / interprebility of the entire function), since duck typing is already practice (not sure how interfaces will work), extending a function from another module without sticking to the established behavior shouldn't happen.

amitmurthy commented 9 years ago

OK. Lets go with removing the export of find then.

tkelman commented 9 years ago

fixed in #37