search

graphql-dotnet / conventions

GraphQL Conventions Library for .NET
MIT License
233 stars 63 forks source link

Add test for data loader sample project #253

Closed Shane32 closed 1 year ago

Shane32 commented 1 year ago

Note: does not actually test Entity Framework

Shane32 commented 1 year ago

@tlil I think these changes constitute most of what I had previously mentioned. The project could really use xml comments, documentation, and NRT attributes, but I don't think I'll be making further changes at this time. I'm also not really sure when we should release develop (aka v8). Note that releasing the changes as v8 throws off the major version number from GraphQL.NET, which doesn't really matter of course. There is also the option of releasing all these new changes as a new minor version of v7 such as 7.3.0, and just assuming that very few people have upgraded to v7.

image

I'm also not sure if you intend to make any further changes, either to v7 or to v8.

FYI, when/if ready to release the develop branch, I would manually merge develop into master (not using a PR so as to preserve the commit history), and then issue the release from GitHub from the master branch. Of course the release could be performed against any branch, but this way the master branch is always the most recent release.

tlil commented 1 year ago

@tlil I think these changes constitute most of what I had previously mentioned. The project could really use xml comments, documentation, and NRT attributes, but I don't think I'll be making further changes at this time. I'm also not really sure when we should release develop (aka v8). Note that releasing the changes as v8 throws off the major version number from GraphQL.NET, which doesn't really matter of course. There is also the option of releasing all these new changes as a new minor version of v7 such as 7.3.0, and just assuming that very few people have upgraded to v7.

:+1: Makes sense - thanks again for all of these changes. Good stuff!

I'm also not sure if you intend to make any further changes, either to v7 or to v8.

I am – will keep you posted.

FYI, when/if ready to release the develop branch, I would manually merge develop into master (not using a PR so as to preserve the commit history), and then issue the release from GitHub from the master branch. Of course the release could be performed against any branch, but this way the master branch is always the most recent release.

Coolio – will do.

Shane32 commented 1 year ago

Oh last comment: there is a decent chance that some of the GraphQL.NET IGraphQLBuilder methods are not compatible with the conventions project, such as AddAutoClrMappings and anything else related to the actual GraphQL types. You'd probably have a much better idea than me. I'd suggest documenting any known incompatibilities in the readme file. But most stuff, such as UseApolloTracing, works perfect.

Ref: https://graphql-dotnet.github.io/docs/getting-started/dependency-injection#dependency-injection-registration-helpers

Shane32 commented 1 year ago

Oh, and you might want to add a section on authorization to the readme. The GraphQL.NET Server project contains the authorization rule with samples for JWT, subscriptions, etc -- but the proper metadata needs to be added to the graph types.

tlil commented 1 year ago

Oh last comment: there is a decent chance that some of the GraphQL.NET IGraphQLBuilder methods are not compatible with the conventions project, such as AddAutoClrMappings and anything else related to the actual GraphQL types. You'd probably have a much better idea than me. I'd suggest documenting any known incompatibilities in the readme file. But most stuff, such as UseApolloTracing, works perfect.

Thanks for the callout; I'll take a closer look at that to verify what works and what doesn't - and as you suggest, document it accordingly in the README file.