Skip to content
This repository has been archived by the owner. It is now read-only.

Move Pretzel.Logic to multitarget net451/netstandard2.0 #328

Merged
merged 48 commits into from Sep 17, 2019

Conversation

biohazard999
Copy link
Contributor

@biohazard999 biohazard999 commented Sep 5, 2019

#320

  • This add's support for netstandard20 and net451 for the Pretzel.Logic project.
  • Cause MEF is not supported in netstandard20 use System.Composition (MEF2) instead.

This is of course an breaking change for Plugin Authors.

@biohazard999 biohazard999 marked this pull request as ready for review Sep 6, 2019
@laedit
Copy link
Member

@laedit laedit commented Sep 7, 2019

Thanks a lot!

It appears that GitVersion isn't included in Visual Studio 2019 images, so can you edit the appveyor.yml and add the following before the use of GitVersion?

  - choco install gitversion.portable

I will try to review the PR during the wee-end.

@biohazard999
Copy link
Contributor Author

@biohazard999 biohazard999 commented Sep 7, 2019

Thanks a lot!

It appears that GitVersion isn't included in Visual Studio 2019 images, so can you edit the appveyor.yml and add the following before the use of GitVersion?

  - choco install gitversion.portable

Seams there is an issue on GitVersion on VS2019 image so

  - choco install gitversion.portable --version 4.0.0

Did the trick.
Now I need to tackle xunit...

I will try to review the PR during the wee-end.

Thanks! :)

@biohazard999
Copy link
Contributor Author

@biohazard999 biohazard999 commented Sep 7, 2019

@laedit Hmmm could not get the build to working. but I'm not sure whats failing...

@laedit
Copy link
Member

@laedit laedit commented Sep 9, 2019

Sorry, I didn't get the time to check it this weekend.

The build is weird, I don't see either why it is failing.
Can you try to comment the first line?

$ErrorActionPreference = "Stop"

Maybe it will show more info.

.gitignore Outdated Show resolved Hide resolved
BuildScripts/AppVeyor-Build.ps1 Outdated Show resolved Hide resolved
BuildScripts/AppVeyor-Build.ps1 Outdated Show resolved Hide resolved
@laedit
Copy link
Member

@laedit laedit commented Sep 10, 2019

@biohazard999 Sorry to bother you but in order to avoid mixing all things, I will create another PR dedicated to the build fix.
Could you remove any modification related to it in this PR?
Thanks.

@biohazard999
Copy link
Contributor Author

@biohazard999 biohazard999 commented Sep 10, 2019

@biohazard999 Sorry to bother you but in order to avoid mixing all things, I will create another PR dedicated to the build fix.
Could you remove any modification related to it in this PR?
Thanks.

Sure, I'll split it up in two batches if you like.

@laedit
Copy link
Member

@laedit laedit commented Sep 10, 2019

Sure, I'll split it up in two batches if you like.

No need to, just keep the multi-targets here and I handle the build with #330

@biohazard999
Copy link
Contributor Author

@biohazard999 biohazard999 commented Sep 16, 2019

Ok for the public properties, but yes please revert the classes when possible, thanks.

Configuration was the only class that was internal and exported. So that should be fixed

src/Pretzel/Program.cs Outdated Show resolved Hide resolved
src/Pretzel/Program.cs Outdated Show resolved Hide resolved
@laedit
Copy link
Member

@laedit laedit commented Sep 16, 2019

So I think we are good here, or am I missing something?
I have started to write a small doc with the public differences (for plugins writers mostly) can you tell me if I have forgotten something?
Thanks!

@biohazard999
Copy link
Contributor Author

@biohazard999 biohazard999 commented Sep 16, 2019

So I think we are good here, or am I missing something?
I have started to write a small doc with the public differences (for plugins writers mostly) can you tell me if I have forgotten something?
Thanks!

I think we can overcome this limitation with the convention based model. But for now I think that's it.

You should mention that we moved from

  • System.ComponentModel.Composition to System.Composition v1.2.0 for now.
  • net451 & netstandard2.0 so they should multitarget in the future as well
  • Probably mention as a note that we plan to go net462 to reduce effort.

But I think we are through 😉 Was a hard PR, but I think worth the effort and thanks for your razor sharp eyes 😃

@biohazard999
Copy link
Contributor Author

@biohazard999 biohazard999 commented Sep 17, 2019

@laedit Anything there holing the PR back?

@laedit laedit added this to Review in progress in V1.0 - biohazard Sep 17, 2019
@laedit
Copy link
Member

@laedit laedit commented Sep 17, 2019

Sorry, didn't get the time to do anything on github today.

Thanks a lot for your hard work! 😄

Next tasks before releasing the next version (not necessarily by you :) )

  • target net462 #335
  • replace ScriptCs #336
  • replace other dependencies which doesn't support netstandard #337
  • use Cake as build definition #329
  • make a dotnet global tool #339

V1.0 - biohazard automation moved this from Review in progress to Reviewer approved Sep 17, 2019
laedit
laedit approved these changes Sep 17, 2019
@laedit laedit merged commit 83c8009 into Code52:master Sep 17, 2019
1 check passed
V1.0 - biohazard automation moved this from Reviewer approved to Done Sep 17, 2019
@biohazard999 biohazard999 deleted the topic/netstandard branch Sep 17, 2019
@biohazard999
Copy link
Contributor Author

@biohazard999 biohazard999 commented Sep 17, 2019

No worries, I thought you just forgot to merge it 😁

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants