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

Can we have auto [Expose] so 3rd party classes can be used as traits #10

Open
MathiasReichardt opened this issue Nov 28, 2023 · 8 comments · May be fixed by #11
Open

Can we have auto [Expose] so 3rd party classes can be used as traits #10

MathiasReichardt opened this issue Nov 28, 2023 · 8 comments · May be fixed by #11

Comments

@MathiasReichardt
Copy link

Just found nice generator.

I was wondering if the [Expose] attribute is really necessary. It adds clarity when owning the trait class but on the other hand we can not use external classes.
Maybe opt-out via enum e.g.:
[UseTrait(typeof(ABase), AutoExpose=Expose.PropertiesAndMethods)]

So far works like a charm. THX!

@Grauenwolf
Copy link
Contributor

Ok, that was a pain in the ass. Apparently EVERYTHING is a method when you get down to the ISymbol level. Getters and setters, event handlers, even constructors.

I don't know if I should praise or damn you for forcing me to learn so much more about Roslyn.

I posted version 0.10.0 up on NuGet. Let me know how it works.

@Grauenwolf Grauenwolf linked a pull request Nov 28, 2023 that will close this issue
@MathiasReichardt
Copy link
Author

MathiasReichardt commented Nov 28, 2023

Thank your for you fast implementation! Hope i can stay on the praise side of things :)

I tested boldly with MemoryStream:

using Tortuga.Shipwright;

namespace Traits.Examples;

[UseTrait(typeof(MemoryStream), AutoExpose = Expose.All)]
public partial class StreamComposition
{
    public StreamComposition()
    {
    }
}

And have found two issues:

  1. The out keyword is not transferred for TryGetBuffer(out ArraySegment<byte> buffer)
  2. A function wrapper for protected override void Dispose(bool disposing) of MemoryStream is generated. Seems like the access modifier is not taken into account. After that the Dispose() of Stream is not used for the wrapper. Additional, I would not be able to extend the dispose behavior since the original Dispose() of Stream is not virtual. Exactly a problem which is solved by [Expose] I guess since it would allow me to manually implement it.

Both issues lead to a compilation error in generated class.

However, for my current needs I am more happy with what I got.
Still wanted to give some feedback for your kind effort.

THX again 👍

@Grauenwolf
Copy link
Contributor

That makes sense because we assume the person putting on the [Expose] attribute is checking the visibility.

So I think we can fix this by setting it to only auto-expose public methods.

@Grauenwolf
Copy link
Contributor

P.S. Feedback helps a lot to keep me motivated.

@MathiasReichardt
Copy link
Author

So I think we can fix this by setting it to only auto-expose public methods.

That makes sense to me. And since I am interested in composition I am not supposed to fiddle around with anything that is protected. At least for that use-case.

One way out of the "Dispose()-Issue" could be something like [ExcludeAutoExpose(nameof(Stream.Dispose()))]. This way an implementation could be done manually, taking into account all Traits that need disposing.
Smells like the diamond problem. Going down a rabbit hole...

@Grauenwolf
Copy link
Contributor

I just dropped 0.11.0 with support for MemoryStream's output parameters.

@Grauenwolf
Copy link
Contributor

One way out of the "Dispose()-Issue" could be something like [ExcludeAutoExpose(nameof(Stream.Dispose()))]. This way an implementation could be done manually, taking into account all Traits that need disposing.

Yea, that's something that will be needed eventually. But it's going to require more brainpower than I have to get the API right.

@MathiasReichardt
Copy link
Author

Can confirm that MemoryStream works now. This is a great new feature.

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 a pull request may close this issue.

2 participants