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

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

Closed
wants to merge 12 commits into from

Conversation

kfcampbell
Copy link
Member

@kfcampbell kfcampbell commented Feb 26, 2024

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 Test projects end up with this GUID microsoft/vstest#472 (comment))
  • Remove <LangVersion>7.2</LangVersion> on test projects
  • Fix Run_Specifies_Cancellation_Token which (rightfully) throws when running on .NET 6

0xced and others added 3 commits August 19, 2023 01:26
* Update test projects from .NET Core 3.1 (out of support) to .NET 6 (LTS)
* Remove `<Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />`  (see microsoft/vstest#472 (comment))
* Remove `<LangVersion>7.2</LangVersion>` on test projects
* Fix `Run_Specifies_Cancellation_Token` which (rightfully) throws when running on .NET 6
Copy link

👋 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! 🚀

@kfcampbell kfcampbell changed the title Revive cleanup projects PR feat: breaking change: revive cleanup projects PR, target .NET 6.0 Feb 26, 2024
@kfcampbell
Copy link
Member Author

4661572 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.

Assert.Equal(StripWhitespace(expectedString), StripWhitespace(actualString));
// hacky fix for anonymous types: actual strings give "new" and expected strings give "new object"
expectedString = StripWhitespace(expectedString);
expectedString = expectedString.Replace("new{", "newobject{");
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this feels like something is off with serialization. I'm not sure what the proper call would be here. Let me pull this down today and see if I can get a better picture of what might be going on.

@nickfloyd nickfloyd added Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels Mar 4, 2024
@nickfloyd
Copy link
Contributor

Closing in favor of: #321

@nickfloyd nickfloyd closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants