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

Target framework to net-standard 2.0 instead of "net6.0" #46

Merged
merged 4 commits into from
Oct 29, 2021

Conversation

hey24sheep
Copy link
Contributor

In response to issue #45, Target framework ported from "net6.0" to "netstandard2.1"

@aloisdg aloisdg added enhancement New feature or request hacktoberfest Hacktoberfest encourages participation in the open source community, which grows bigger every year. hacktoberfest-accepted labels Oct 21, 2021
@aloisdg aloisdg linked an issue Oct 21, 2021 that may be closed by this pull request
Copy link
Member

@aloisdg aloisdg left a comment

Choose a reason for hiding this comment

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

Good job

@hey24sheep
Copy link
Contributor Author

Thanks

@torendil
Copy link
Collaborator

As discussed in the issue, if we target .netstandard 2.1, we are leaving out all .net framework apps that cannot use .netstandard 2.1. I'm fine with it, but I feel like we are missing the plot in here if we do that ;)

Can you downgrade it to 2.0 so that the library becomes compatible with .net framework please? (still referencing https://docs.microsoft.com/fr-fr/dotnet/standard/net-standard)

@hey24sheep
Copy link
Contributor Author

Hi, I didn't check the compatibility, sorry. I have downgraded it to 2.0.

Fixed this error on netstandard2.0 also. Let me know if any other changes are needed, thanks.

code_cap
.

@torendil
Copy link
Collaborator

Oh, actually, I'm pretty much sure we need that default implementation for the logic of things.
@aloisdg, do you confirm? By the way, it seems that the tests in the runner are simply built but not ran?

If we actually need that default implementation, let's then keep it to .netstandard 2.1, it would be sad to pass on the opportunity to use modern features of the language.
@hey24sheep I'm sorry for all this back and forth, I don't mean to make you work for nothing :'(

@hey24sheep
Copy link
Contributor Author

hey24sheep commented Oct 26, 2021

@torendil Hi, it's okay but I have a question

Oh, actually, I'm pretty much sure we need that default implementation for the logic of things.

Why? your abstract base implements a default implementation which is same as the interface for your abstract class and overriden implementation (with LuhnCheck) in aluhn class.

I don't mind changing it back, but I am curious if I am missing something here.

@torendil
Copy link
Collaborator

Why? your abstract base implements a default implementation which is same as the interface for your abstract class and overriden implementation (with LuhnCheck) in aluhn class.

I don't mind changing it back, but I am curious if I am missing something here.

No, it is just me forgetting the details :) It is actually ok, my bad :)

As .net framework is still very prevalent, I believe it is better to stick with 2.0 at the cost of a few features.

There is still the problem with the tests that don't run in this PR. Can you please try to run them locally @hey24sheep?

@thinkbeforecoding
Copy link
Member

It is possible to have multiple platform in a single nuget.
So it would be possible to have a netstandard2.0 for compatibility with .Net framework, and net5.0/net6.0 to target most recent features.

@aloisdg
Copy link
Member

aloisdg commented Oct 27, 2021

Multi targeting is what we did on Cardizer. I guess we can go this road <TargetFrameworks>net5.0;net472</TargetFrameworks> in our case we will have <TargetFrameworks>net6.0;netstandard2_0</TargetFrameworks>

Here is what I have in mind

TargetFramework(s) Library Test
net6.0 ✔️ ✔️
netstandard2_0 ✔️

AFAIK net5.0 should handle netstandard2_0

source and plural

@aloisdg aloisdg mentioned this pull request Oct 27, 2021
@hey24sheep
Copy link
Contributor Author

I have updated the target framework, test are running now. Do you want me to update the nuget packages as well?

@torendil
Copy link
Collaborator

I have updated the target framework, test are running now. Do you want me to update the nuget packages as well?

You can, I'm not sure it is required but you can.
Tell me what you want to do, depending on that we can merge

@hey24sheep
Copy link
Contributor Author

It is not needed, but nuget update we can do as a separate issue because thats a separate thing. I don't want to break anything also :D

Copy link
Collaborator

@torendil torendil left a comment

Choose a reason for hiding this comment

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

Nice

@torendil torendil merged commit 8b90e07 into d-edge:main Oct 29, 2021
@torendil torendil changed the title Target framework to net-standard 2.1 instead of "net6.0" Target framework to net-standard 2.0 instead of "net6.0" Oct 29, 2021
@aloisdg
Copy link
Member

aloisdg commented Oct 29, 2021

Why did the Tests project target netstandard2.0 again?

@hey24sheep
Copy link
Contributor Author

Why did the Tests project target netstandard2.0 again?

You said to multi target in your above response. So I added a multi target.

@aloisdg
Copy link
Member

aloisdg commented Oct 31, 2021

We have a new error: #60

@hey24sheep hey24sheep deleted the feat/port_to_netstandard2 branch November 10, 2021 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest Hacktoberfest encourages participation in the open source community, which grows bigger every year. hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Target multiple sdks lower than .net6.0
4 participants