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

[Breaking] Scoping generic extension methods #1959

Merged
merged 2 commits into from Oct 30, 2023
Merged

Conversation

Fydar
Copy link
Contributor

@Fydar Fydar commented Oct 17, 2023

"3rd PR and I'm already suggesting breaking changes; I hope this isn't too intrusive, but I considered this quite high priority."

Adding a generic scope to unscoped generic extension methods.

PR Details

Description

This means that these extension methods no longer work with IntPtr and IReferencable types, however this feature was unused internally and the engine continues to compile like normal.

Should we be interested in using a similar extension method, separate extension methods for IReferencable types could be created, potentially without "Dispose" in the name, and instead use the "Release" naming associated with IReferencable types.

Related Issue

  • None

Motivation and Context

This was causing every object to have additional methods appearing in the IDE and on the documentation, which can be very confusing for new developers and irritating for experienced ones.

Everywhere in the documentation are notes about this extension method being usable, despite it being meaningless in the majority of use cases.

image

And it is also appears on every object, regardless of type.

image

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@manio143
Copy link
Member

This means that these extension methods no longer work with IntPtr and IReferencable types

I got a little confused as to why this is mentioned if no code changes were necessary in relation to those types.
I see it's based on this code

public T Add<T>([NotNull] T objectToDispose)
{
if (!(objectToDispose is IDisposable || objectToDispose is IntPtr || objectToDispose is IReferencable))
throw new ArgumentException("Argument must be IDisposable, IReferenceable or IntPtr");

The reason why those methods are generic over T is because they seemed to be used in an initialization pattern

var smth = new Thing().DisposeBy(collector);
  • Adding an overload for IntPtr makes sense.

But there's no way to overload on T where it's either IDisposable or IReferencable.

  • I would suggest adding
    T ReleaseBy<T>(this T reference, ICollectionHolder holder) where T : IReferencable;

This was causing every object to have additional methods appearing in the IDE and on the documentation, which can be very confusing for new developers and irritating for experienced ones.

This means that these extension methods no longer work with IntPtr and "IReferencable" types, but this feature was unused.

Seperate extension methods for "IReferencable" types could be created without "Dispose" in the name and instead use the "Release" naming.
Adding ReleaseBy and RemoveReleaseBy to replace the lost functionality of DisposeBy.

Fixed typos and overly-aggressive copy pasting in documentation comments.
@Eideren
Copy link
Collaborator

Eideren commented Oct 30, 2023

Did you test this change out in editor/in game ? I don't really have time right now to check

@Fydar
Copy link
Contributor Author

Fydar commented Oct 30, 2023

Building the solution works, building and running the editor works.

@Eideren Eideren merged commit 680f493 into stride3d:master Oct 30, 2023
1 check passed
@Eideren
Copy link
Collaborator

Eideren commented Oct 30, 2023

Thanks !

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.

None yet

3 participants