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

Add support for netstandard2.0 #51

Merged
merged 3 commits into from Aug 16, 2019

Conversation

macsux
Copy link
Contributor

@macsux macsux commented Aug 16, 2019

No description provided.

Copy link
Collaborator

@SteveSyfuhs SteveSyfuhs left a comment

Choose a reason for hiding this comment

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

This PR is great. Couple minor questions, but otherwise I'm happy with it.

Kerberos.NET/Kerberos.NET.Simple.csproj Outdated Show resolved Hide resolved
Kerberos.NET/NetStandardExtensions.cs Show resolved Hide resolved
@macsux
Copy link
Contributor Author

macsux commented Aug 16, 2019

It would be good to add a way to test both branches of code in the unit test, as I can't easily tell which branch of code unit tests go down... Thoughts?

@SteveSyfuhs
Copy link
Collaborator

It occurs to me that perhaps we don't even need netcoreapp2.1 as part of the project. I'm not sure what benefits are gained by having both, since netcoreapp2.1 will happily work with standard?

That would at least simplify testing greatly.

@macsux
Copy link
Contributor Author

macsux commented Aug 16, 2019

I didn't wanna fully switch to netstandard because netcoreapp methods are more efficient as they take direct use of memory object where i had to do array copying in a few places. But honestly in context of getting a token, that's an optimization that won't affect anything in meaningful way so I'm happy to just target netstandard20 and drop netcoreapp to keep things simple

@SteveSyfuhs
Copy link
Collaborator

Let's cut netcoreapp2.1 then. I'm not confident the original implementation that uses the new stuff is written correctly as far as performance goes anyway. :)

@macsux
Copy link
Contributor Author

macsux commented Aug 16, 2019

Should be good to merge now. I ripped out netcoreapp21 dependency

@SteveSyfuhs SteveSyfuhs merged commit d2d21e8 into dotnet:master Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants