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

Support IrrationalConstants 0.2 #44

Merged
merged 6 commits into from
Jun 30, 2023
Merged

Support IrrationalConstants 0.2 #44

merged 6 commits into from
Jun 30, 2023

Conversation

devmotion
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Patch coverage has no change and project coverage change: -3.58 ⚠️

Comparison is base (acc9bac) 95.23% compared to head (dcd9eca) 91.66%.

❗ Current head dcd9eca differs from pull request most recent head 7bc3c8a. Consider uploading reports for the commit 7bc3c8a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
- Coverage   95.23%   91.66%   -3.58%     
==========================================
  Files           1        1              
  Lines          21       12       -9     
==========================================
- Hits           20       11       -9     
  Misses          1        1              
Impacted Files Coverage Δ
src/Tau.jl 91.66% <0.00%> (-3.58%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

test/runtests.jl Outdated
@@ -2,7 +2,7 @@ using Tau
using Test

@testset "self-identity" begin
@test tau isa Irrational{:twoπ}
@test tau isa Tau.IrrationalConstants.Twoπ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking: this works with both IrrationalConstants 0.1 and 0.2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, no, the test doesn't. Of course, the package/constant works with either version but it's a different type since IrrationalConstants changed the types. I guess a better test might be based on twoπ directly:

Suggested change
@test tau isa Tau.IrrationalConstants.Twoπ
@test tau === Tau.IrrationalConstants.twoπ

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it, I think it's better now @giordano.

The question made me wonder whether we should be also make a breaking release, as we did in IrrationalConstants, just to be sure that the different type does not break dispatches of some user. It seems less likely though, at least according to Juliahub no packages depend on Tau.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have the following two possibilities.

  • Do not consider this PR as breaking, and update the compat table with 0.1, 0.2.
  • Consider this PR as breaking, and update the compat table with 0.2.

The current behaviour in this PR

  • Consider this PR as breaking, and update the compat table with 0.1, 0.2.

is inconsistent because users may experience surprising type changes in the following situation.

  • The user depends on Tau.jl v2.0.0.
  • The user depends on SomePkg.jl that depends on IrrationalConstants.jl.
  • SomePkg.jl updates its compat table of IrrationalConstants.jl from 0.1 to 0.1, 0.2 without a breaking change.
  • Then, the type of Tau.tau will be changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I guess you're right and it's safer to drop support for IrrationalConstants 0.1.

@devmotion
Copy link
Member Author

Let's wait with the PR and a breaking release until #46 is released and possibly #43 is fixed.

Project.toml Outdated
@@ -1,12 +1,12 @@
name = "Tau"
uuid = "c544e3c2-d3e5-5802-ac44-44683f340e4a"
version = "1.0.0"
version = "1.1.0"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be on the safe side since types change and hence possibly dispatches are broken:

Suggested change
version = "1.1.0"
version = "2.0.0"

@devmotion devmotion merged commit 2b2bd4e into master Jun 30, 2023
@devmotion devmotion deleted the dw/irrationalconstants branch June 30, 2023 13:09
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.

3 participants