Skip to content
This repository was archived by the owner on Jul 6, 2026. It is now read-only.

[Java.Intreop] Rename Dispose methods - #238

Merged
jonpryor merged 1 commit into
dotnet:masterfrom
radekdoulik:pr-rename-dispose-methods
Jan 3, 2018
Merged

[Java.Intreop] Rename Dispose methods#238
jonpryor merged 1 commit into
dotnet:masterfrom
radekdoulik:pr-rename-dispose-methods

Conversation

@radekdoulik

Copy link
Copy Markdown
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)

@jonpryor

Copy link
Copy Markdown
Contributor

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
radekdoulikforce-pushed the pr-rename-dispose-methods branch from b188f6e to b8bd979CompareJanuary 3, 2018 13:45
@radekdoulik

Copy link
Copy Markdown
MemberAuthor

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

@jonpryor

Copy link
Copy Markdown
Contributor

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
radekdoulikforce-pushed the pr-rename-dispose-methods branch from b8bd979 to 10a18efCompareJanuary 3, 2018 15:12
@radekdoulik

Copy link
Copy Markdown
MemberAuthor

Updated.

@jonpryor
jonpryor merged commit 19379a3 into dotnet:masterJan 3, 2018
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 14, 2024
Sign up for freeto 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

@radekdoulik@jonpryor