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

Fixed open/closed interval definitions #232

Merged
merged 5 commits into from Sep 24, 2022
Merged

Fixed open/closed interval definitions #232

merged 5 commits into from Sep 24, 2022

Conversation

g-as
Copy link
Contributor

@g-as g-as commented Sep 22, 2022

Fixes #231

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2022

Codecov Report

Merging #232 (ff61e37) into main (d823871) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #232   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          859       859           
  Branches       175       175           
=========================================
  Hits           859       859           
Impacted Files Coverage Δ
src/phantom/interval.py 100.00% <100.00%> (ø)
src/phantom/predicates/interval.py 100.00% <100.00%> (ø)
src/phantom/sized.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@antonagestam
Copy link
Owner

Hi @g-as, thanks for putting in the time to address is!

I'm again pondering the issue of backwards compatibility. I'm now thinking it would make sense to rename the predicate functions and the types to use terms "inclusive" and "exclusive". In my opinion those names are more intuitive, but it would also solve the issue of backwards compatibility.

Just to clarify a bit about my reasoning here, I'll expand a little bit. In releasing a version 1.0, I'm not too worried about removing a function or a class. If someone fails to read the release notes, it's still a change that should be easily discovered by a test suite and probably in 99% of usages this will break at import-time. OTH, renaming closed -> open and vice versa requires a much more disciplined test suite to discover and I'm worried it will sail under the radar into production code before discovered.

What do you think? If you agree it's a good idea, please feel free to go ahead and make those changes on this PR.

Thanks again for the work thus far! 🙏

@g-as
Copy link
Contributor Author

g-as commented Sep 24, 2022

Agreed that breakage is the best way to avoid behaviour change without the user's knowledge. So renaming with appropriate names, and arguably better/more explicit names is the best way to go.

Changes incoming asap.

@antonagestam antonagestam merged commit 0bae3c8 into antonagestam:main Sep 24, 2022
@antonagestam
Copy link
Owner

Thank you so much for the help, and again for making aware of this 🍰

@g-as
Copy link
Contributor Author

g-as commented Sep 24, 2022

My pleasure.

Keep up the good work.

@g-as g-as deleted the bug/intervals branch September 24, 2022 22:36
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

Successfully merging this pull request may close these issues.

Intervals Closed/Open backwards??
3 participants