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

Drop support for .NET 4.6 #2521

Merged
merged 12 commits into from
Aug 4, 2022
Merged

Drop support for .NET 4.6 #2521

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c6ed5df
Cleaning up compiler warnings
nickfloyd Aug 3, 2022
90234a3
Converts projects to only use .NET Standard 2.0
nickfloyd Aug 3, 2022
65ffc44
Cleans up the cake/build/ci warnings buy updating the obsolete methods
nickfloyd Aug 3, 2022
c211ee8
Update Octokit.Reactive/Octokit.Reactive.csproj
nickfloyd Aug 3, 2022
bdafb9f
updates to use the singular node
nickfloyd Aug 3, 2022
559b4fa
updates cake APIs to use the non depricated ones
nickfloyd Aug 3, 2022
3ca846f
corrects typo in namespace
nickfloyd Aug 3, 2022
02116bd
implements the correct method via namespace
nickfloyd Aug 3, 2022
d7314bf
includes a version of System.Reactive that a .net core 3.1 app can use
nickfloyd Aug 3, 2022
98ac272
adds CopyLocalLockFileAssemblies so that System.Reactive gets copied …
nickfloyd Aug 3, 2022
87b25e5
removes obsolite constructor and tests.
nickfloyd Aug 4, 2022
db363f5
patches up tests
nickfloyd Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 2 additions & 12 deletions Octokit.Reactive/Octokit.Reactive.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<Authors>GitHub</Authors>
<Version>0.0.0-dev</Version>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<TargetFrameworks>netstandard2.0;net46</TargetFrameworks>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
nickfloyd marked this conversation as resolved.
Show resolved Hide resolved
<AssemblyName>Octokit.Reactive</AssemblyName>
<PackageId>Octokit.Reactive</PackageId>
<DebugType>embedded</DebugType>
Expand All @@ -19,11 +19,8 @@
<Copyright>Copyright GitHub 2017</Copyright>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
<NetStandardImplicitPackageVersion>2.0.0</NetStandardImplicitPackageVersion>
</PropertyGroup>

<PropertyGroup>
<NetStandardImplicitPackageVersion>2.0.0</NetStandardImplicitPackageVersion>
nickfloyd marked this conversation as resolved.
Show resolved Hide resolved
<NoWarn>1591;1701;1702;1705</NoWarn>
</PropertyGroup>

Expand All @@ -44,11 +41,4 @@
<PackageReference Include="System.Reactive" Version="4.4.1" />
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.1.1" PrivateAssets="All"/>
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'net46' ">
<Reference Include="System" />
<Reference Include="Microsoft.CSharp" />
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.2" PrivateAssets="All" />
</ItemGroup>

</Project>
15 changes: 8 additions & 7 deletions Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


var updatedRepository = await _github.Repository.Edit(repoContext.RepositoryOwner, repoContext.RepositoryName, update);

Expand All @@ -550,7 +550,8 @@ public async Task UpdatesNameWithRepositoryIdObsolete()
using (var repoContext = await _github.CreateUserRepositoryContext())
{
var updatedName = Helper.MakeNameWithTimestamp("updated-repo");
var update = new RepositoryUpdate(updatedName);
var update = new RepositoryUpdate() { Name = updatedName };


var updatedRepository = await _github.Repository.Edit(repoContext.RepositoryId, update);

Expand Down Expand Up @@ -811,7 +812,7 @@ public async Task UpdatesMergeMethodWithRepositoryId()
Assert.True(editedRepository.AllowAutoMerge);
}
}

[IntegrationTest]
public async Task UpdatesDeleteBranchOnMergeMethod()
{
Expand All @@ -826,7 +827,7 @@ public async Task UpdatesDeleteBranchOnMergeMethod()
Assert.True(repository.DeleteBranchOnMerge);
}
}

[IntegrationTest]
public async Task UpdatesDeleteBranchOnMergeMethodWithRepositoryId()
{
Expand Down Expand Up @@ -1047,7 +1048,7 @@ public async Task ReturnsSpecifiedRepositoryWithLicenseInformation()
Assert.Equal("mit", repository.License.Key);
Assert.Equal("MIT License", repository.License.Name);
}

[IntegrationTest]
public async Task ReturnsRepositoryDeleteBranchOnMergeOptions()
{
Expand Down Expand Up @@ -2099,10 +2100,10 @@ public async Task ReturnsCodeOwnersErrors()
using (var repoContext = await _github.CreateUserRepositoryContext())
{
await _github.Repository.Content.CreateFile(repoContext.RepositoryOwner, repoContext.RepositoryName, ".github/codeowners", new CreateFileRequest("Create codeowners", @"* [email protected]"));

// Sometimes it takes a second to create the file
Thread.Sleep(TimeSpan.FromSeconds(2));

var license = await _github.Repository.GetAllCodeOwnersErrors(repoContext.RepositoryOwner, repoContext.RepositoryName);
Assert.NotEmpty(license.Errors);
}
Expand Down
18 changes: 2 additions & 16 deletions Octokit/Octokit.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<Authors>GitHub</Authors>
<Version>0.0.0-dev</Version>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<TargetFrameworks>netstandard2.0;net46</TargetFrameworks>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<AssemblyName>Octokit</AssemblyName>
<PackageId>Octokit</PackageId>
<DebugType>embedded</DebugType>
Expand All @@ -19,26 +19,12 @@
<Copyright>Copyright GitHub 2017</Copyright>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
<PropertyGroup>
<DefineConstants>$(DefineConstants);HAS_TYPEINFO;SIMPLE_JSON_INTERNAL;SIMPLE_JSON_OBJARRAYINTERNAL;SIMPLE_JSON_READONLY_COLLECTIONS;SIMPLE_JSON_TYPEINFO</DefineConstants>
<NetStandardImplicitPackageVersion>2.0.0</NetStandardImplicitPackageVersion>
</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

</PropertyGroup>

<PropertyGroup>
<NoWarn>1591;1701;1702;1705</NoWarn>
</PropertyGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'net46' ">
<Reference Include="System.Net.Http" />
<Reference Include="System" />
<Reference Include="Microsoft.CSharp" />
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.2" PrivateAssets="All" />
</ItemGroup>

<ItemGroup>
<None Include="images\octokit.png" Pack="true" PackagePath="\" />
</ItemGroup>
Expand Down
2 changes: 1 addition & 1 deletion build/Tasks/ValidateLINQPadSamples.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public override void Run(Context context)
.Combine("Octokit.Reactive")
.Combine("bin")
.Combine(context.Configuration)
.Combine("net46")
.Combine("netstandard2.0")
.MakeAbsolute(context.Environment)
.FullPath;

Expand Down