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

Deprecate the function ctrl.Finish() #50

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Deprecate the function ctrl.Finish() #50

merged 1 commit into from
Sep 5, 2023

Conversation

alexandear
Copy link
Contributor

@alexandear alexandear commented Aug 4, 2023

This PR marks the function ctrl.Finish as Deprecated. It's no need to call this function starting from Go 1.14 when T.Cleanup was added.

@fifsky
Copy link

fifsky commented Aug 4, 2023

If Finish is marked as Deprecated, a large amount of code that has already been written will not pass the golangci-lint check, which will take a long time to transform

@bcho
Copy link
Contributor

bcho commented Aug 4, 2023

If Finish is marked as Deprecated, a large amount of code that has already been written will not pass the golangci-lint check, which will take a long time to transform

I want to echo this. Instead of removing the Finish call directly, can we do a graceful deprecation? For example, we make this Finish call as no-op in the next version (may also print a log when calling this), then remove it in the next major release? This could provide a smoother migrate experience to the user.

@Nivl
Copy link

Nivl commented Aug 4, 2023

I want to echo this. Instead of removing the Finish call directly, can we do a graceful deprecation? For example, we make this Finish call as no-op in the next version (may also print a log when calling this), then remove it in the next major release? This could provide a smoother migrate experience to the user.

It's not being removed, it's being marked as deprecated.

If Finish is marked as Deprecated, a large amount of code that has already been written will not pass the golangci-lint check, which will take a long time to transform

This is fine and expected. People need to configure their linter correctly and plan their work accordingly.

Here's the issue. Deprecations are just notices that exist because removing a method would introduce a breaking change and people need time to update. A deprecation notice gives them the time they need. Now if we start using tools that break everything if we have a deprecation, that would make the whole deprecation thing useless.

@sywhang sywhang requested a review from r-hang August 15, 2023 18:28
@marten-seemann
Copy link
Contributor

Please don't remove this function. It's needed to use gomock with certain testing frameworks, and removing the function would break those use cases.

Please also consider reverting the deprecation. If the function is not going to be removed in a future release, it shouldn't be marked deprecated in the first place.

See #81 for more details.

@alexandear alexandear deleted the deprecated-ctrl-finish branch November 16, 2023 14:44
sywhang added a commit to sywhang/mock that referenced this pull request Jul 10, 2024
Finish no longer has to be explicitly called on Controller with defer
if the Controller was created with testing.T.

We tried to just mark this as deprecated in uber-go#50 but doing so would break
many users, so we reverted it in uber-go#85.

In doing so we dropped some wordings from the doc that marked Finished
as unnecessary for most cases.

This PR fixes the docstring to guide users without explicitly marking
the API deprecated.
sywhang added a commit that referenced this pull request Jul 10, 2024
Finish no longer has to be explicitly called on Controller with defer if
the Controller was created with testing.T.

We tried to just mark this as deprecated in #50 but doing so would break
many users, so we reverted it in #85.

In doing so we dropped some wordings from the doc that marked Finished
as unnecessary for most cases.

This PR fixes the docstring to guide users without explicitly marking
the API deprecated.
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.

7 participants