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

GraphQL 4.0.0 Bump. Project clean up. #80

Merged
merged 12 commits into from
Apr 15, 2021

Conversation

JoeHartzell
Copy link
Contributor

This includes a bump to the GraphQL version for 4.0.0. It also includes cleaning up the example projects. This would include

GraphQL.Relay.StarWars

  • Remove Node.js dependency
  • Fix swapi url
  • Bump GraphQL version to 4.0.0.
  • Bump dotnet version to .net5

GraphQL.Relay

  • Bump GraphQL version to 4.0.0.
  • Bump dotnet version to .net5
  • Fixed breaking changes

GraphQL.Relay.Todo

  • Bump GraphQL version to 4.0.0.
  • Bump dotnet version to .net5
  • Fixed breaking changes

GraphQL.Relay.Test

  • Bump GraphQL version to 4.0.0.
  • Bump dotnet version to .net5
  • Fixed breaking changes

Would resolve a few open issues. #79, #54

@Shane32
Copy link
Member

Shane32 commented Mar 29, 2021

This is a big step in the right direction; thanks @JoeHartzell ! Once the "TODO: Test this" comment has been resolved, I would approve this. I think the project still has a lot more cleanup to do before it is ready for another release.

@JoeHartzell
Copy link
Contributor Author

Alright. I will look into adding a test for that. In the little bit of testing I did, it seems to work. I just didn't get to verifying it with a test case.

I did see quite a bit more to clean up around documentation, example projects, etc... If you have a roadmap/issues that need resolved I would be more than happy to help. I was trying really hard to just keep this focused on the .net/graphql version updates. 😂

@sungam3r
Copy link
Member

Relates to #55

@Shane32
Copy link
Member

Shane32 commented Mar 30, 2021

Yes, much easier to review multiple smaller PRs than one large one!

The next step (separate PR) is probably stripping away all the bits of the library that don’t relate to Relay and/or have other better implementations, such as deleting the Http namespace in favor of the server project. (I see you’ve already converted the sample projects to use the server project 👍 .)

@JoeHartzell
Copy link
Contributor Author

Alright I wrote a few test cases for the remaining TODO. Turns out that the method I used was incorrect, should be corrected now.
Since this relates to #55, would you like me to merge those changes into this PR?

@sungam3r
Copy link
Member

No. I need to review both independently.

src/GraphQL.Relay/GraphQL.Relay.csproj Outdated Show resolved Hide resolved
@JoeHartzell
Copy link
Contributor Author

Alright I think this is good to go. I'm not sure why this is conflicting? Those conflicts should have been resolved at commit b3ad664. The conflict showed up after accepting the suggestion change to the .net targeting.

There is nothing for me to merge from the upstream (GraphQL.Relay master) branch.

@Shane32
Copy link
Member

Shane32 commented Mar 31, 2021

Switch to the upstream branch and pull it, and then switch back to your branch and merge from master, then push the merge commit. I think @sungam3r pulled in some dependabot changes within the last 24 hours which caused the conflict.

@Shane32
Copy link
Member

Shane32 commented Mar 31, 2021

If you don't have the upstream master branch pulled, then you need to "fetch" the upstream repo so that when you try to merge, it will see the new commits.

@Shane32
Copy link
Member

Shane32 commented Mar 31, 2021

Merge this JoeHartzell#1 and it will fix the PR @JoeHartzell

@Shane32
Copy link
Member

Shane32 commented Mar 31, 2021

Switch to the upstream branch and pull it, and then switch back to your branch and merge from master, then push the merge commit. I think @sungam3r pulled in some dependabot changes within the last 24 hours which caused the conflict.

Btw, since your branch is named master, you won't be able to pull the main repo's master branch. But you should still be able to "fetch" the main repo and then merge changes from it.

@JoeHartzell
Copy link
Contributor Author

Alright thanks for the clarification there!

@sungam3r
Copy link
Member

sungam3r commented Apr 1, 2021

I would like to review this before merge when I have time.

@Shane32
Copy link
Member

Shane32 commented Apr 1, 2021

@sungam3r If you have time, that would be great! This isn't the end-all of cleanup required for this repo before we release another version, so if you are busy, then we should merge it based on my review. We can certainly wait a bit though.

@Shane32
Copy link
Member

Shane32 commented Apr 13, 2021

@sungam3r I have reviewed this PR and am going to merge this to keep development moving on this repo. Mostly this is just cleanup and transition from 2.4.0 anyway.

@Shane32 Shane32 merged commit eae38b7 into graphql-dotnet:master Apr 15, 2021
@Shane32
Copy link
Member

Shane32 commented Apr 15, 2021

@sungam3r I've merged this PR. Please make comments when/if you have time to do so, and we can address them in a separate PR.

@Shane32
Copy link
Member

Shane32 commented Apr 15, 2021

This is the first meaningful commit to this repo in almost 3 years. 🥇

using GraphQL.Server;
using GraphQL.Types;
using GraphQL.Types.Relay;
using GraphQL.Relay.Types;
Copy link
Member

Choose a reason for hiding this comment

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

Add empty line after using.

@@ -19,7 +19,7 @@ public void Message_WhenMessageIsProvidedInCtor_ReturnsPassedMessage()
var ex = new IncompleteSliceException("Test");
Assert.Equal("Test", ex.Message);
ex = new IncompleteSliceException("Test", "paramName");
Assert.StartsWith($"Test{Environment.NewLine}", ex.Message);
Assert.StartsWith($"Test", ex.Message);
Copy link
Member

Choose a reason for hiding this comment

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

Remove $

@@ -32,7 +32,7 @@ public void InnerException_WhenInnerExceptionIsPassedToCtor_ReturnsPassedExcepti
Assert.Equal(inner, ex.InnerException);

ex = new IncompleteSliceException("Test", "paramName", inner);
Assert.StartsWith($"Test{Environment.NewLine}", ex.Message);
Assert.StartsWith($"Test", ex.Message);
Copy link
Member

Choose a reason for hiding this comment

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

Remove $

@@ -0,0 +1,119 @@
namespace GraphQL.Relay.Test.Types
{
using System.Linq;
Copy link
Member

Choose a reason for hiding this comment

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

Move usings outside the namespace.

{
public string Id { get; set; }

public static IEnumerable<SimpleData> GetData() => new List<SimpleData> {
Copy link
Member

Choose a reason for hiding this comment

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

{ on new line

public override SimpleData GetById(string id) => SimpleData
.GetData()
.FirstOrDefault(x => x.Id.Equals(id));

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line


public class SimpleSchema : Schema
{
public SimpleSchema() => (Query) = (new QueryGraphType());
Copy link
Member

Choose a reason for hiding this comment

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

public SimpleSchema() => (Query) = (new QueryGraphType());
->
public SimpleSchema() => Query = new QueryGraphType();

{
public TodoMutation(): base() {
public TodoMutation() : base()
Copy link
Member

Choose a reason for hiding this comment

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

public TodoMutation() : base() -> public TodoMutation()

@sungam3r
Copy link
Member

I've merged this PR. Please make comments when/if you have time to do so, and we can address them in a separate PR.

#86

@JoeHartzell
Copy link
Contributor Author

JoeHartzell commented Apr 16, 2021

I think @Shane32 might have addressed some of these. As most of these are "linting" errors, do you think it would make sense to add an .editorconfig file to the project? Could probably just bring the one over from graphql-dotnet just to keep things consistent?

@sungam3r
Copy link
Member

Yes

@sungam3r
Copy link
Member

As an option we may pull editorconfig from main repo into this repo root when building on ci.

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.

3 participants