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

Avoid extending Base.isopen #100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Avoid extending Base.isopen #100

wants to merge 1 commit into from

Conversation

omus
Copy link
Collaborator

@omus omus commented Jun 4, 2020

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 omus added the breaking label Jun 4, 2020
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:

```julia
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.
@oxinabox
Copy link
Member

oxinabox commented Jun 4, 2020

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.


@omus
Copy link
Collaborator Author

omus commented Jun 4, 2020

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.

@oxinabox
Copy link
Member

oxinabox commented Jun 4, 2020

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.

@codecov
Copy link

codecov bot commented Jun 4, 2020

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%) ⬇️

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.

@omus
Copy link
Collaborator Author

omus commented Jun 4, 2020

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

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

Seems like an unnecessary inconsistency

@iamed2
Copy link
Member

iamed2 commented Jun 5, 2020

@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.

@oxinabox
Copy link
Member

oxinabox commented Jun 5, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants