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

[Java.Intreop] Rename Dispose methods #238

Merged
merged 1 commit into from
Jan 3, 2018

Conversation

radekdoulik
Copy link
Member

These have different signatures than the common dispose pattern
methods, thus rename them to avoid confusion.

Catched by gendarme's
https://github.com/spouliot/gendarme/wiki/Gendarme.Rules.Design.UseCorrectDisposeSignaturesRule(2.10)

@radekdoulik radekdoulik requested a review from jonpryor January 2, 2018 15:53
@jonpryor
Copy link
Member

jonpryor commented Jan 2, 2018

As mentioned on our last call, if we're going to rename JniValueManager.Dispose(IJavaPeerable) to JniValueManager.DisposePeer(IJavaPeerable), then we should also append a Peer suffix all the other verbs for consistency within JniValueManager, e.g.

  • Add(IJavaPeerable) becomes AddPeer(IJavaPeerable)
  • Remove(IJavaPeerable) becomes RemovePeer(IJavaPeerable)
  • Finalize(IJavaPeerable) becomes FinalizePeer(IJavaPeerable)
  • etc.

Thus, a question: which is "better", Add()/Remove()/Finalize()/etc. or AddPeer()/RemovePeer()/FinalizePeer()/etc.? If we don't like the Peer suffix, we should just ignore the Dispose() warning in Gendarme.

@radekdoulik radekdoulik force-pushed the pr-rename-dispose-methods branch from b188f6e to b8bd979 Compare January 3, 2018 13:45
@radekdoulik
Copy link
Member Author

Indeed, I misremembered it as belonging to another fix. Updated.

@jonpryor
Copy link
Member

jonpryor commented Jan 3, 2018

We should also rename Collect() to CollectPeers().

These have different signatures than the common dispose pattern
methods, thus rename them to avoid confusion.

Also rename other methods working with IJavaPeerable to be consistent.

Catched by gendarme's
https://github.com/spouliot/gendarme/wiki/Gendarme.Rules.Design.UseCorrectDisposeSignaturesRule(2.10)
@radekdoulik radekdoulik force-pushed the pr-rename-dispose-methods branch from b8bd979 to 10a18ef Compare January 3, 2018 15:12
@radekdoulik
Copy link
Member Author

Updated.

@jonpryor jonpryor merged commit 19379a3 into dotnet:master Jan 3, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants