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.

Sign up for GitHub

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

Jump to bottom

Drop support for .NET 4.6 #2521

Merged
merged 12 commits into from
Aug 4, 2022
Merged

Drop support for .NET 4.6 #2521

merged 12 commits into from
Aug 4, 2022

Conversation

nickfloyd
Copy link
Contributor

@nickfloyd nickfloyd commented Aug 3, 2022 •
edited
Loading

Fixes: #2500

Already covered by the explanation via @JamieMagee - this needs to be done so we can move forward with the framework and innovate using the new APIs and assemblies.

Highlights from the issue description:

.NET Framework 4.6 (and 4.6.1) went out of support on April 26, 2022. Continuing to publish Octokit for this target adds a support burden.

.NET standard 2.0 covers .NET Framework 4.6.2 already.

Supporting a single target framework, .NET standard 2.0, would simplify maintenance of this project while supporting all target frameworks currently supported upstream.

For reference, these are the supported versions going forward

.NET implementation Version support
.NET and .NET Core 2.0, 2.1, 2.2, 3.0, 3.1, 5.0, 6.0
.NET Framework  4.6.1 2, 4.6.2, 4.7, 4.7.1, 4.7.2, 4.8

nickfloyd
nickfloyd commented Aug 3, 2022
View reviewed changes
Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs Outdated
@@ -536,7 +536,7 @@ public async Task UpdatesNameObsolete()
using (var repoContext = await _github.CreateUserRepositoryContext())
{
var updatedName = Helper.MakeNameWithTimestamp("updated-repo");
var update = new RepositoryUpdate(updatedName);
var update = new RepositoryUpdate() { Name = updatedName };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addressed a compiler warning - I wanted to ensure the output was as clean as possible and, therefore, clean a little bit of the house regardless of whether it was a related change or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These two tests were there to test the obsolete method, which technically does still work. The previous 2 tests in the file check the newer way of doing it, so if we don't want the warnings it would be better to get rid of those two tests (ending Obsolete) entirely 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I'm thinking we also opt to remove the constructor as well given we are making a breaking change with this change set - and I can note it in the release.

@nickfloyd nickfloyd marked this pull request as ready for review August 3, 2022 20:16
@nickfloyd
adds CopyLocalLockFileAssemblies so that System.Reactive gets copied …
98ac272 
…during CI - since no .net framework version is specified
@nickfloyd
Copy link
Contributor Author

Packages inspected from CI.

Before Octokit
after-octokit

After Octokit
before-octokit

Before Octokit.Reactive
before

After Octokit.Reactive
after

@nickfloyd nickfloyd changed the title WIP: Drop support for .NET 4.6 Drop support for .NET 4.6 Aug 3, 2022
@nickfloyd nickfloyd requested a review from JamieMagee August 3, 2022 20:59
@nickfloyd
Copy link
Contributor Author

cc: @eriawan @JonruAlveus

JonruAlveus
JonruAlveus reviewed Aug 4, 2022
View reviewed changes
Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs Outdated
@@ -536,7 +536,7 @@ public async Task UpdatesNameObsolete()
using (var repoContext = await _github.CreateUserRepositoryContext())
{
var updatedName = Helper.MakeNameWithTimestamp("updated-repo");
var update = new RepositoryUpdate(updatedName);
var update = new RepositoryUpdate() { Name = updatedName };
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two tests were there to test the obsolete method, which technically does still work. The previous 2 tests in the file check the newer way of doing it, so if we don't want the warnings it would be better to get rid of those two tests (ending Obsolete) entirely 👍

Octokit/Octokit.csproj
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net46' ">
<DefineConstants>$(DefineConstants);HAS_ENVIRONMENT;HAS_REGEX_COMPILED_OPTIONS;SIMPLE_JSON_INTERNAL;SIMPLE_JSON_OBJARRAYINTERNAL;SIMPLE_JSON_READONLY_COLLECTIONS;HAS_SERVICEPOINTMANAGER</DefineConstants>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are some usages of these constants in the code, should these be removed/tidied as well as part of this?

For example HAS_ENVIRONMENT has been removed entirely but is present/used in Connection.cs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good highlight. I can clean these up as well while I am in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scratch that. There are implementations in the other projects that I'd like to take a closer look at - this is feeling like a separate changeset. I'm going to hold off so that we can do this separately - #2526

@nickfloyd nickfloyd requested a review from JonruAlveus August 4, 2022 14:06
JonruAlveus
JonruAlveus approved these changes Aug 4, 2022
View reviewed changes
Copy link
Collaborator

@JonruAlveus JonruAlveus left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me!

@nickfloyd nickfloyd merged commit 6e72fda into main Aug 4, 2022
@nickfloyd nickfloyd deleted the drop-net46-support branch August 4, 2022 17:12
@eriawan
Copy link
Contributor

eriawan commented Aug 4, 2022

@nickfloyd

Sorry to join late! Thanks for this PR, now we can move to newer .NET Framework version! 👍

@nickfloyd nickfloyd mentioned this pull request Aug 4, 2022
Closed
@nickfloyd
Copy link
Contributor Author

release_notes: Drops support for .NET 4.6 and updates all projects to .NET Standard 2.0

@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 and removed category: breaking-change labels Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonruAlveus JonruAlveus JonruAlveus approved these changes

@JamieMagee JamieMagee JamieMagee approved these changes

Assignees
No one assigned
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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop support for .NET Framework 4.6
4 participants
@nickfloyd @eriawan @JamieMagee @JonruAlveus