octokit.graphql.net icon indicating copy to clipboard operation
octokit.graphql.net copied to clipboard

feat: breaking change: revive cleanup projects PR, target .NET 6.0

Open kfcampbell opened this issue 4 months ago • 2 comments

BREAKING CHANGE: target .NET 6.0.

Builds the project on .NET 8. Also makes the following changes of @0xced's from #304:

  • Update test projects from .NET Core 3.1 (out of support) to .NET 6 (LTS)
  • Remove <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" /> (see https://github.com/Microsoft/vstest/issues/472#issuecomment-378811616)
  • Remove <LangVersion>7.2</LangVersion> on test projects
  • Fix Run_Specifies_Cancellation_Token which (rightfully) throws when running on .NET 6

kfcampbell avatar Feb 26 '24 17:02 kfcampbell

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

github-actions[bot] avatar Feb 26 '24 17:02 github-actions[bot]

https://github.com/octokit/octokit.graphql.net/pull/313/commits/4661572495ec86fd03b540e800cc727a93157b26 has a test "fix" for a serialization issue that occurs when updating .NET versions. Essentially, the expected string query string in tests looks like (note the x => new piece):

data => Rewritten.Value.Select(
    data["data"]["repository"]["issue"],
    x => new
    {
        Value = Rewritten.Value.SingleOrDefault(
            Rewritten.Value.Select(
                x["value"],
                y => new
                {
                    Closed = y["closed"].ToObject<bool>(),
                    Description = y["description"].ToObject<string>()
                }))
    })

and the actual string looks like (note the x => new object piece):

data => Rewritten.Value.Select(
    data["data"]["repository"]["issue"],
    x => new object
    {
        Value = Rewritten.Value.SingleOrDefault(
            Rewritten.Value.Select(
                x["value"],
                y => new object
                {
                    Closed = y["closed"].ToObject<bool>(),
                    Description = y["description"].ToObject<string>()
                }))
    })

These are the "same" content, but fail string comparisons. I'm not sure how to monkey with our string comparison library or our query generation to get these to be truly identical. @StanleyGoldman, since you originally introduced this comparison mechanism years ago, I'd be very interested to know if you have thoughts!

In the meantime, my hacky "fix" is a find/replace that looks for instances of "new{" in the expected string and replaces them with "newobject{" to match the actual strings. I don't love this approach, hence I'm leaving this PR in draft for the time being.

@nickfloyd and @0xced, I'm also interested to hear if you have comments on this.

kfcampbell avatar Feb 27 '24 21:02 kfcampbell