Small code/readme cleanup + CI fix #62

sungam3r · 2019-09-21T23:08:45Z

[UPDATE] See #87 first.

~~fixes #50~~
~~fixes #61~~
~~heap allocation optimizations~~
~~null argument checks~~
~~add comments~~
~~formatting~~
~~remove MVC from example~~

fixes graphql-dotnet#61 heap allocation optimizations null argument checks add comments formatting remove MVC from example

sungam3r · 2019-09-21T23:12:53Z

I apologize for the big PR. I just wanted to look at this repository for a long time and finally I was able to do it. Also, I expect to support this project in the future, because my team uses it in the production.

sgabler-solytic · 2019-11-19T16:27:14Z

@sungam3r This looks really useful. If I understand the current state of this PR correctly, I should already be able to follow your improved Readme to setup authorization for my project without any breaking changes, right?

sungam3r · 2019-11-19T16:54:56Z

Well, you may try. Basically, everything should work.

iamkoch · 2019-11-27T11:43:27Z

First of all, thanks for raising this one. The broken README.md was starting to cause me nightmares.

One thing, though: In the README.md, the call to AddGraphQLAuth is wrong.

You have

 .AddGraphQLAuth(settings, =>

but it should be the following, including the IServiceProvider?

.AddGraphQLAuth((settings, provider) =>

more cleanup

joemcbride · 2019-11-27T15:09:55Z

@iamkoch the README could say .AddTeddyBears() and it would not be wrong. That is just an example of what YOU are supposed to write in your own project. So it is whatever YOU decide it is.

sungam3r · 2019-11-27T15:11:15Z

@iamkoch There are two overloads, I use overload with one parameter, but in the readme gave an example of overload with two parameters. Fixed. I updated GraphQL dependency, there were some changes in the api.

sungam3r · 2019-11-27T15:13:16Z

I need to find time to get back to this PR and make sure that it is ready for merge.

README.md

joemcbride

As I’ve stated in other PRs, there are a ton of formatting changes that are not needed and not required. If you’re fixing bugs or adding features please do those as separate PRs so they can be independently reviewed.

sungam3r · 2019-11-27T15:34:38Z

I will make small focused PRs.

sungam3r · 2019-11-27T15:39:59Z

@joemcbride In general, the main thing that once stopped me from continuing to work on this PR is a similar package for authorization in the server project. I saw that they are very similar. I understand that second project uses a specific ASP.NET Core auth api. Nevertheless, there was a desire to understand both and, if possible, bring them to the same denominator. Ideally, I would like the solution when there would be no copied code at all between the two projects. Do you think it's worth the effort?

joemcbride · 2019-11-27T15:46:17Z

@sungam3r maybe - though the intent of this project was so that it would not be ASP.NET
Core specific, so it could also be used in other non-ASP.NET projects and ASP.NET 4.5 projects. Others essentially copied what was here, made it ASP.NET specific, and added it to the server project.

iamkoch · 2019-11-27T18:28:50Z

If the sample code is invalid or doesn't compile then why bother adding sample code? The PR looks to address inconsistencies in the sample code borne out of changes to the main repo, so imo I'm warranted in raising a very minor issue about its correctness.

…

On Wed, 27 Nov 2019, 15:09 Joe McBride, ***@***.***> wrote: @iamkoch <https://github.com/iamkoch> the README could say .AddTeddyBears() and it would not be wrong. That is just an example of what YOU are supposed to write in your own project. So it is whatever YOU decide it is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#62?email_source=notifications&email_token=AAGBMDKV2WG4GPHYFAQ46B3QV2EUJA5CNFSM4IZAKZO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFJZSTY#issuecomment-559126863>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGBMDOSQ4BCRTQOEPVZKMTQV2EUJANCNFSM4IZAKZOQ> .

sungam3r · 2019-11-27T19:16:58Z

Sample code is valid and compiles. Talk about which version of the example would be better to show.

sungam3r · 2020-10-29T23:02:09Z

This PR will be revised after #84

… cleanup # Conflicts: # README.md # appveyor.yml # src/GraphQL.Authorization.Tests/AuthenticatedUserRequirementTests.cs # src/GraphQL.Authorization.Tests/AuthorizationEvaluatorTests.cs # src/GraphQL.Authorization.Tests/AuthorizationSchemaBuilderTests.cs # src/GraphQL.Authorization.Tests/AuthorizationSettingsTests.cs # src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs # src/GraphQL.Authorization.Tests/ClaimAuthorizationRequirementTests.cs # src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj # src/GraphQL.Authorization.Tests/ValidationTestBase.cs # src/GraphQL.Authorization.sln # src/GraphQL.Authorization/AuthorizationEvaluator.cs # src/GraphQL.Authorization/AuthorizationMetadataExtensions.cs # src/GraphQL.Authorization/AuthorizationPolicyBuilder.cs # src/GraphQL.Authorization/AuthorizationValidationRule.cs # src/GraphQL.Authorization/GraphQL.Authorization.csproj # src/GraphQL.Authorization/Requirements/ClaimAuthorizationRequirement.cs # src/Harness/GraphQLAuthExtensions.cs # src/Harness/Harness.csproj # src/Harness/Program.cs # src/Harness/Properties/launchSettings.json # src/Harness/Startup.cs

… cleanup # Conflicts: # src/GraphQL.Authorization.Tests/AuthorizationEvaluatorTests.cs # src/GraphQL.Authorization/AuthorizationPolicy.cs # src/GraphQL.Authorization/AuthorizationPolicyBuilder.cs # src/GraphQL.Authorization/AuthorizationResult.cs # src/GraphQL.Authorization/AuthorizationSettings.cs # src/GraphQL.Authorization/IAuthorizationPolicy.cs # src/GraphQL.Authorization/IProvideClaimsPrincipal.cs # src/GraphQL.Authorization/Requirements/ClaimAuthorizationRequirement.cs # src/GraphQL.Authorization/Requirements/IAuthorizationRequirement.cs

.github/workflows/test.yml

… cleanup # Conflicts: # src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.approved.txt # src/GraphQL.Authorization.Tests/AuthenticatedUserRequirementTests.cs # src/GraphQL.Authorization.Tests/AuthorizationSchemaBuilderTests.cs # src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs # src/GraphQL.Authorization/AuthorizationContext.cs # src/GraphQL.Authorization/AuthorizationEvaluator.cs # src/GraphQL.Authorization/AuthorizationMetadataExtensions.cs # src/GraphQL.Authorization/AuthorizationSettings.cs

… cleanup # Conflicts: # README.md # src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.approved.txt # src/GraphQL.Authorization/AuthorizationContext.cs # src/GraphQL.Authorization/AuthorizationEvaluator.cs # src/GraphQL.Authorization/AuthorizationSettings.cs # src/GraphQL.Authorization/IAuthorizationEvaluator.cs # src/Harness/GraphQL.cs

sungam3r · 2021-03-22T08:44:27Z

@Shane32 Meet our old friend here

sungam3r · 2021-03-22T09:35:13Z

Regarding labeler: graphql-dotnet/graphql-dotnet#2439

fixes graphql-dotnet#50

ab8c0c1

fixes graphql-dotnet#61 heap allocation optimizations null argument checks add comments formatting remove MVC from example

sungam3r mentioned this pull request Oct 16, 2019

Defining multiple schemas graphql-dotnet/graphql-dotnet#1154

Closed

sungam3r changed the title ~~Code cleanup~~ Code cleanup [WIP] Oct 21, 2019

update for ValidateAsync

b010870

more cleanup

joemcbride reviewed Nov 27, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

joemcbride requested changes Nov 27, 2019

View reviewed changes

sungam3r added 3 commits November 1, 2020 17:21

fixes

49981a9

revert

9cae8a3

sungam3r mentioned this pull request Nov 1, 2020

Small cleanup #87

Merged

Shane32 reviewed Mar 6, 2021

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

sungam3r added 5 commits March 22, 2021 11:03

revert

af25ea7

fix readme

7ad7179

1

1c5918b

sungam3r requested a review from Shane32 March 22, 2021 08:41

sungam3r changed the title ~~Code cleanup [WIP]~~ Code cleanup Mar 22, 2021

fix build

043c111

sungam3r changed the title ~~Code cleanup~~ Small code/readme cleanup + CI fix Mar 22, 2021

fix labeler?

d378c41

sungam3r added this to the 4.0 milestone Mar 22, 2021

sungam3r self-assigned this Mar 22, 2021

revert

f5f0fd2

Shane32 approved these changes Mar 22, 2021

View reviewed changes

sungam3r added documentation enhancement New feature or request labels Mar 22, 2021

sungam3r merged commit 46eaa4c into graphql-dotnet:master Mar 22, 2021

sungam3r deleted the cleanup branch March 22, 2021 17:09

Uh oh!

Small code/readme cleanup + CI fix #62

Small code/readme cleanup + CI fix #62

Uh oh!

Conversation

sungam3r commented Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sungam3r commented Sep 21, 2019

Uh oh!

sgabler-solytic commented Nov 19, 2019

Uh oh!

sungam3r commented Nov 19, 2019

Uh oh!

iamkoch commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joemcbride commented Nov 27, 2019

Uh oh!

sungam3r commented Nov 27, 2019

Uh oh!

sungam3r commented Nov 27, 2019

Uh oh!

Uh oh!

joemcbride left a comment

Choose a reason for hiding this comment

Uh oh!

sungam3r commented Nov 27, 2019

Uh oh!

sungam3r commented Nov 27, 2019

Uh oh!

joemcbride commented Nov 27, 2019

Uh oh!

iamkoch commented Nov 27, 2019 via email

Uh oh!

sungam3r commented Nov 27, 2019

Uh oh!

sungam3r commented Oct 29, 2020

Uh oh!

Uh oh!

sungam3r commented Mar 22, 2021

Uh oh!

sungam3r commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sungam3r commented Sep 21, 2019 •

edited

Loading

iamkoch commented Nov 27, 2019 •

edited

Loading