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

v1.0 #728

Closed
wants to merge 2 commits into from
Closed

v1.0 #728

wants to merge 2 commits into from

Conversation

dlfivefifty
Copy link
Contributor

I also updated the CI to test on lts and bumped the julia version

@dlfivefifty dlfivefifty mentioned this pull request Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.00%. Comparing base (c310fb5) to head (08c8f02).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #728      +/-   ##
==========================================
- Coverage   89.57%   89.00%   -0.58%     
==========================================
  Files          11       11              
  Lines         969      973       +4     
==========================================
- Hits          868      866       -2     
- Misses        101      107       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -17,7 +17,7 @@ jobs:
fail-fast: false
matrix:
version:
- '1.6'
- 'lts'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- 'lts'
- 'min'
- 'lts'

(They are not identical)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's min?

Copy link
Member

Choose a reason for hiding this comment

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

It tests the minimum compatible version.

@@ -33,7 +33,7 @@ NaNMath = "1"
Preferences = "1"
SpecialFunctions = "1, 2"
StaticArrays = "1.5"
julia = "1.6"
julia = "1.10"
Copy link
Member

@devmotion devmotion Dec 6, 2024

Choose a reason for hiding this comment

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

Why? Is it broken on < 1.10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but if we aren't testing on Julia v1.6 it's best to explicitly drop support. It also allows us to remove weakdeps as deps and use any features from v1.10+.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that the package is tested successfully on 1.6, it's that the PR in its current form dropped tests on 1.6 (but adding min would fix that). Releasing 1.0 doesn't require making any changes to the package or its dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I'm not generally opposed to making such changes but I think they should be done separately. Releasing 1.0 should just involve updating the version number + removing deprecations (if existent) IMO. Otherwise it's also easy to forget to remove VERSION-specific code, old workarounds, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really care, push any changes you want to this PR

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't want to. I want to keep them in separate PRs. And I don't think any of the changes in this PR apart from the 1.0 bump are necessary for releasing 1.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then make your own PR

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@dlfivefifty dlfivefifty closed this Dec 7, 2024
@dlfivefifty dlfivefifty deleted the v1 branch December 7, 2024 08:10
@dlfivefifty dlfivefifty restored the v1 branch December 7, 2024 08:11
@dlfivefifty dlfivefifty deleted the v1 branch December 7, 2024 08:11
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.

2 participants