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

All: delete outdated deprecations #16330

Merged
merged 6 commits into from Apr 30, 2024
Merged

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Apr 25, 2024

Similar to last time: #15393

Not that much this time.

@erik-krogh erik-krogh marked this pull request as ready for review April 26, 2024 10:57
@erik-krogh erik-krogh requested review from a team as code owners April 26, 2024 10:57
aschackmull
aschackmull previously approved these changes Apr 26, 2024
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Java 👍
I love that you keep doing these! Have you got the process fully automated by now?

@erik-krogh
Copy link
Contributor Author

Java 👍 I love that you keep doing these! Have you got the process fully automated by now?

The initial deletion of the predicates has been automated the entire time.
But I still need to look through the changes and delete any excess dead code, and then make sure everything still works.
And the change-note is still manual.

alexrford
alexrford previously approved these changes Apr 26, 2024
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

Ruby 👍

tausbn
tausbn previously approved these changes Apr 26, 2024
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Python 👍

@@ -0,0 +1,4 @@
---
category: minorAnalysis
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be breaking?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this has been discussed in the past, sorry if that's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is technically a breaking change to delete code. However, it's code that has been deprecated for more than a year, so external users have had plenty of to migrate.

So I still think it's a minor change.

I couldn't find a discussion in the past about this.
But all previous change-notes when deleting deprecations have been minorAnalysis. (Since the first deprecation deletion PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the amount of time the thing has been deprecated should influence whether or not this is a breaking change. A breaking change is a breaking change.

Obviously, this breaking change is totally justified, but I still think stuff like this should be marked as a breaking change going forward

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've changed it to breaking.
I usually look at the previous PR when making the next one, so I should mark these as breaking in the future.

jketema
jketema previously approved these changes Apr 26, 2024
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

C++ 👍

hvitved
hvitved previously approved these changes Apr 26, 2024
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

C# 👍

Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Go 👍🏻

@erik-krogh erik-krogh merged commit 7e83979 into github:main Apr 30, 2024
46 checks passed
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.

None yet

9 participants