Intervals.jl icon indicating copy to clipboard operation
Intervals.jl copied to clipboard

Avoid extending Base.isopen

Open omus opened this issue 5 years ago • 7 comments

The isopen function defined here is very different and so it seems best we define a new function rather than extend Base.isopen. Doing this means we need to export Intervals.isopen to make this change non-breaking however doing this results in:

WARNING: both Intervals and Base export "isopen"; uses of it in module Main must be qualified
ERROR: UndefVarError: isopen not defined

It then seems best to not export Intervals.isopen and also then not export Intervals.isclosed to keep things consistent.

As this change is breaking it should be included in the next major release and we can postpone merging this until we want to make a major release.

omus avatar Jun 04 '20 18:06 omus

Alternative. We swap to using is_open and is_closed Which follow the style guide. and we can explort them as they don't conflict with Base.

Also we can deprecate Base.isopen and Base.isclosed to them.


oxinabox avatar Jun 04 '20 19:06 oxinabox

Alternative. We swap to using is_open and is_closed Which follow the style guide. and we can explort them as they don't conflict with Base.

Also we can deprecate Base.isopen and Base.isclosed to them.

Very true we are free to name these functions however we want. That said having both is_open and isopen functions available does seem like a strange thing to do. I think keeping the names as isopen and isclosed is probably the right approach here.

omus avatar Jun 04 '20 19:06 omus

That said having both is_open and isopen functions available does seem like a strange thing to do.

True, but on the other hand, Base.isopen is used so rarely that I forgot it existed. Because normally for open things you know they are open as you openned them (and are using open(...) do.) Thus I suspect it would be less confusing, since I think others like me also just don't see isopen being used that much.

oxinabox avatar Jun 04 '20 19:06 oxinabox

Codecov Report

Merging #100 into master will increase coverage by 0.09%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
+ Coverage   96.55%   96.64%   +0.09%     
==========================================
  Files           7        7              
  Lines         261      328      +67     
==========================================
+ Hits          252      317      +65     
- Misses          9       11       +2     
Impacted Files Coverage Δ
src/Intervals.jl 100.00% <ø> (ø)
src/inclusivity.jl 100.00% <100.00%> (ø)
src/interval.jl 97.09% <100.00%> (-0.05%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 99abb65...ba7ca39. Read the comment docs.

codecov[bot] avatar Jun 04 '20 20:06 codecov[bot]

Another example where is_open would be worse is if someone would write:

is_open(interval1) && issubset(interval1, interval2)

Seems like an unnecessary inconsistency

omus avatar Jun 04 '20 20:06 omus

@oxinabox https://github.com/search?q=isopen+language%3Ajulia&type=Code

Also isopen is often used when you're not sure whether something else has closed the thing.

iamed2 avatar Jun 05 '20 05:06 iamed2

in our intental codebase, we use isopen exactly once from what I can tell. Though to be fair most of our IOish stuff is open source.

But this also likely means that this change to Intervals to use a different isopen is probably not going to break much, so easy upgrading

oxinabox avatar Jun 05 '20 08:06 oxinabox