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

v0.14-DEV: Consolidate nomenclature to IntegrationRule #82

Merged
merged 15 commits into from
Sep 24, 2024

Conversation

mikeingold
Copy link
Owner

Completed

  • Rename IntegrationAlgorithm to IntegrationRule (breaking)
  • Update all old references from "algorithm", "settings", etc to "rule"

Copy link
Collaborator

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

Thanks! I have only one minor suggestion below. What is your plan regarding the branches? Do you want to merge this into main or into another branch as you say in this PR?

src/integral.jl Outdated Show resolved Hide resolved
src/integral_aliases.jl Outdated Show resolved Hide resolved
src/integral_aliases.jl Outdated Show resolved Hide resolved
src/integral_aliases.jl Outdated Show resolved Hide resolved
@mikeingold
Copy link
Owner Author

What is your plan regarding the branches? Do you want to merge this into main or into another branch as you say in this PR?

At first my plan was to create, effectively, a v0.14 branch. Then I'd create sub-branches for sizable tasks and PR those individually into the v0.14 branch. Then, when ready, we'd PR the v0.14 branch into main. As I was working on this task, though, that started to seem a little extravagant. The ultimate PR would still show a diff including all of the sub-PR's contents merged together, so there'd still be some mental accounting to ensure all of the commits came from reviewed PR's or were reviewed at that stage.

What I decided to move towards was just PR'ing these tasks direct to main but with Project.toml version v0.14-DEV. Once all of the planned changes are implemented, we'd just need to drop -DEV from the version and trigger registration. I think the other process above could probably make sense for a huge change (e.g. v1.0 to v2.0), a protracted development cycle, and/or with a much larger user base, but it might just be a little too much overhead in this particular case.

Let me know if that makes sense or if you disagree.

Co-authored-by: Joshua Lampert <51029046+JoshuaLampert@users.noreply.github.com>
@JoshuaLampert
Copy link
Collaborator

Thanks for the detailed explanation. I agree with you and your plan sounds reasonable!

@JoshuaLampert
Copy link
Collaborator

JoshuaLampert commented Sep 24, 2024

One question, however, remains: What do we do with the non-breaking PRs I opened?

  1. Do we want to merge them before this PR (and others containing breaking changes) and then create a new non-breaking release before?
  2. Do we want to merge them after the breaking release and make a new non-breaking release in the v0.14 cycle?
  3. Do we just include them in the breaking release?
  4. Do you think they are not even worth merging?

I plan to create one more PR including tests using ExplicitImports.jl if you agree that this makes sense.

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 94.50549% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.35%. Comparing base (8d5f81a) to head (c8d7bda).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/specializations/Ring.jl 50.00% 2 Missing ⚠️
src/specializations/Rope.jl 50.00% 2 Missing ⚠️
src/specializations/BezierCurve.jl 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #82   +/-   ##
=======================================
  Coverage   90.35%   90.35%           
=======================================
  Files          16       16           
  Lines         311      311           
=======================================
  Hits          281      281           
  Misses         30       30           

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

@mikeingold
Copy link
Owner Author

One question, however, remains: What do we do with the non-breaking PRs I opened?

I just went ahead and merged the downgrade PR. That looked non-conflicting and should still be merge-able with this one. Since there's really no functional changes, just updated compat bounds, I don't know if it's worth pushing another v0.13 release. We can probably just keep rolling toward a v0.14.0 release that incorporates all of these changes.

I like the idea of the JuliaFormatter PR but had a comment/question on which particular code style to enforce, so it might be better to re-run the suggested changes and put that PR in line after this one.

@JoshuaLampert
Copy link
Collaborator

Ok, so this means we go with option 3. from above 👍 This means we can merge this PR, I guess.

@mikeingold mikeingold merged commit 41c5dfe into main Sep 24, 2024
13 checks passed
@mikeingold mikeingold deleted the breaking-rules branch September 24, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants