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

#471 Nested classes are not discovered by source generator #472

Merged
merged 4 commits into from
Aug 26, 2023

Conversation

ekspedycja
Copy link
Contributor

In current implementation of the Source Generator nested classes are not discovered properly. This may be an issue in case of some of Vertical Slices approaches where feature is a class that consists of multiple subclasses.

public static partial class GetAssignedTasks
{
    public record Result()
    {
        public string ResultBlahBlah => "BlahBlahBlah";
    }

    public record Query()
        : TeamRequest<Result>
    {
    }

    public class Validator : AbstractValidator<Query>
    {
        public Validator()
        {
            RuleFor(p => p.GameId).NotEmpty();
            RuleFor(p => p.TeamId).NotEmpty();
            RuleFor(p => p.TeamName).NotEmpty();
        }
    }
    public partial class Endpoint : BaseTeamEndpoint<Query, Result, Result>
    {
        public override void Configure()
        {
            Get("assignedTasks");
        }
    }
}

Its not an issue with Endpoint and its inheritance as when class is moved outside the top level class it is discovered properly.

The PR also simplifies the approach with discovering namespaces - IMO current approach is the way around.

@nefarius
Copy link
Member

nefarius commented Aug 24, 2023

Could you enable allowing edits by maintainers please? Trying to commit a minor style fix, thx. See below.

};

public void Initialize(IncrementalGeneratorInitializationContext context)
public void Initialize(IncrementalGeneratorInitializationContext context)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you eliminate the one redundant space at the start, thanks 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dj-nitehawk
Copy link
Member

@nefarius

is this PR ok to be merged other than the space thing?
i didn't look at it properly, but saw that the exclusion list was removed.
is that alright?

@nefarius
Copy link
Member

nefarius commented Aug 24, 2023

The exclusion list got removed; the matching of derived classes is now based on searching for the interfaces including the FastEndpoints namespace, I still wanna test it in one of my projects but IMHO unless it is not desired to allow someone to create endpoints in a Microsoft, NuGet etc. namespace it should work alright.

@dj-nitehawk need your brain here; could that lead to any undesired side-effects I forgot about?

@dj-nitehawk
Copy link
Member

yeah that should be alright.
give it a test run in your project and give me the final go ahead for merge.
no rush, next release will be in about a week or so.

@nefarius
Copy link
Member

I have a project where I deliberately had to avoid "NuGet" in my namespaces, this will be perfect to see if I can now do that 😎

@dj-nitehawk
Copy link
Member

@ekspedycja @nefarius

did some testing with this today and it was only discovering public types.
most of my endpoint classes are internal sealed. committed a change that will transform internal types as well.
i'll merge this PR now as it seems to be performing good.

@nefarius if you find any issues in your project, do open up a new issue and we'll look in to it then.

thanks a lot @ekspedycja for this 🙏

@dj-nitehawk dj-nitehawk merged commit 608a16e into FastEndpoints:main Aug 26, 2023
1 check passed
@nefarius
Copy link
Member

@nefarius if you find any issues in your project, do open up a new issue and we'll look in to it then.

Works!

namespace FastEndpoints
{
    public static class DiscoveredTypes
    {
        public static readonly global::System.Type[] All = new global::System.Type[]
        {
            typeof(global::NuGetCachingProxy.Endpoints.CatchallEndpoint),
            typeof(global::NuGetCachingProxy.Endpoints.PackageDownloadEndpoint),
        };
    }
}

👏

nefarius added a commit to nefarius/NuGetCachingProxy that referenced this pull request Aug 26, 2023
@ekspedycja
Copy link
Contributor Author

I'm glad I was able to help 🙂

One thing came to my mind but I had no chance to test it. Currently the list of available types includes FluetValidation.IValidator. I am not quite sure if this may be an issue if some externals packages implement their own Fluent Validators. In the other hand the generator is run on users code not on imported nugets so this should be fine - or maybe a good idea to test that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants