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

Update target frameworks #2131

Merged
merged 3 commits into from Aug 27, 2018

Conversation

Projects
None yet
3 participants
@austindrenski
Member

austindrenski commented Aug 25, 2018

Closes: #2122

@austindrenski austindrenski added this to the 4.1 milestone Aug 25, 2018

@austindrenski austindrenski self-assigned this Aug 25, 2018

@austindrenski austindrenski requested review from roji and YohDeadfall Aug 25, 2018

@YohDeadfall

LGTM

Show outdated Hide outdated src/VSIX/VSIX.csproj Outdated
@@ -7,7 +7,7 @@
<Copyright>Copyright 2018 © The Npgsql Development Team</Copyright>
<Company>Npgsql</Company>
<PackageTags>npgsql postgresql postgres postgis geojson spatial ado ado.net database sql</PackageTags>
<TargetFrameworks>net45;netstandard2.0</TargetFrameworks>
<TargetFrameworks>net452;netstandard2.0</TargetFrameworks>

This comment has been minimized.

@roji

roji Aug 26, 2018

Member

As I wrote elsewhere we may be able to remove .NET Framework as a target of the plugins, since they don't actually need them. We just need to test whether a .NET Framework application referencing npgsql and the plugin properly gets the .NET Framework version of npgsql and the .NET Standard version of the plugin.

But this can be handled in a separate PR.

@roji

roji Aug 26, 2018

Member

As I wrote elsewhere we may be able to remove .NET Framework as a target of the plugins, since they don't actually need them. We just need to test whether a .NET Framework application referencing npgsql and the plugin properly gets the .NET Framework version of npgsql and the .NET Standard version of the plugin.

But this can be handled in a separate PR.

This comment has been minimized.

@austindrenski

austindrenski Aug 26, 2018

Member

Since these changes already required so many modifications due to #if blocks throughout the solution, I figured it would be best to just move everything forward before removing the framework altogether.

Once this is merged, I'll circle back to test/submit that as a separate PR.

@austindrenski

austindrenski Aug 26, 2018

Member

Since these changes already required so many modifications due to #if blocks throughout the solution, I figured it would be best to just move everything forward before removing the framework altogether.

Once this is merged, I'll circle back to test/submit that as a separate PR.

This comment has been minimized.

@roji

roji Aug 26, 2018

Member

Makes sense.

@roji

roji Aug 26, 2018

Member

Makes sense.

@austindrenski

This comment has been minimized.

Show comment
Hide comment
@austindrenski

austindrenski Aug 26, 2018

Member

@roji Updated with checks passing.

Member

austindrenski commented Aug 26, 2018

@roji Updated with checks passing.

@roji

roji approved these changes Aug 27, 2018

LGTM.

Can you please start a 4.1 release notes document mentioning this as a breaking change? We can update it as we commit.

@austindrenski austindrenski added the doc label Aug 27, 2018

@austindrenski austindrenski merged commit 91d23f9 into npgsql:dev Aug 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@austindrenski austindrenski deleted the austindrenski:bump-target-frameworks branch Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment