Skip to content

sendgrid/csharp-http-client#1 Adding port of the http client to Aspne… #2

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

Closed
wants to merge 3 commits into from

Conversation

Gekctek
Copy link

@Gekctek Gekctek commented Apr 19, 2016

…t Core/.Net Core solution/project type

I ported the solution to the new project/solution formats. It targets the net451 and dotnet5.4.

Key points:

  • .Net 451 builds will be exactly the same
  • .Net Core builds require Newtownsoft.Json (no more JsonSeriliaizer or whatever)
  • I left the versioning, license urls, author info alone so none of those are set
  • I could not get NUnit to work with the unit test project so it is now using xUnit (no real difference)
  • The project name has to be the full namespace if you want your dll to be your namespace
  • Tests still run and it still builds the .Net 451 dll
  • I put the .Net Core stuff in the NetCore folder and didnt affect what is currently there so I dont completely destroy anything anyone was working on. But the new solution can replace the old one

@thinkingserious
Copy link
Contributor

Thanks for the pull request @Gekctek!

In order for us to consider it for merging, we will need a signed CLA: https://github.com/sendgrid/csharp-http-client/blob/master/CONTRIBUTING.md#cla

Once that is done, we will review for merging.

With Best Regards,

Elmer

@thinkingserious
Copy link
Contributor

@Gekctek,

Are you still considering signing the CLA?

@Gekctek
Copy link
Author

Gekctek commented Jul 1, 2016

yes sorry. I have been very busy this last month. I will be updating it to the lastest ASpNetCore as well

@Gekctek
Copy link
Author

Gekctek commented Jul 1, 2016

Sorry about the long delay. I have updated my pull request for the .Net Core RTM and have sent in the Agreement forms

@thinkingserious
Copy link
Contributor

Awesome!

Well, now that @Gekctek and @PaitoAnderson have both submitted solutions, it's time to figure out how to get this feature implemented once and for all :)

This issue is not on our current sprint backlog, but it's coming up, so we have some time hash things out. I will facilitate as best as I can.

Since you both are most familiar with what it takes to get ASP.Net Core going, I'm hoping you both may have some ideas on how to move forward. Please advise. Thanks!

@thinkingserious
Copy link
Contributor

Hi @Gekctek,

Could you please repair the merge conflict? Thanks!

@Gekctek
Copy link
Author

Gekctek commented May 4, 2017

Done

@SendGridDX
Copy link

SendGridDX commented May 4, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Gekctek
❌ Ethan Celletti


Ethan Celletti seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Gekctek
Copy link
Author

Gekctek commented May 4, 2017

The Ethan Celletti contributer is me, but I cant sign it a second time. It looks like one of the commits did not have an email attached to it

@thinkingserious
Copy link
Contributor

No problem @Gekctek,

We still have the originally signed CLA from you that has your name. Thanks!

@thinkingserious
Copy link
Contributor

Hi @Gekctek,

I think we should combine your work into the base project rather than creating a separate solution. We did with our SDK and it worked out well.

I know that is significant work and you have already done so much, so I understand if you don't wish to do that. If that is the case, I'll close this PR and create a new one that merges your work with the base project.
What do you think?

Thanks again!

Elmer

@Gekctek
Copy link
Author

Gekctek commented May 18, 2017

Whatever works. I actually dont remember all this because I did this a year ago. But do you mean just change the current files versus adding a new copy of them? If I did that, I would change things a little just to update to use .net core using .csproj rather than the project.json because a lot has happened this past year to .net core

@thinkingserious
Copy link
Contributor

No problem @Gekctek, I'm happy you have been so patient :)

Your PR creates a new folder NetCore and I'm saying that instead of creating a new folder for .NET Core support, we can just add the support to the base project.

@Gekctek
Copy link
Author

Gekctek commented May 24, 2017

So I have had the chance to look at converting the current project over to .Net core. Its definitely doable but I dont know if I could do everything by myself. Such as the build process, assembly signing and nuget packages. I might be able to try to configure them but I wouldnt be able to confirm/test it by myself. So there are a couple options,

  1. I can just convert the code/project structure to the new model, (different from the current because .Net core has changed in the last year) and update the pull request and you guys handle the rest
  2. I try to attempt changing everything, but I dont guarantee success because I am not familiar with some of your systems
  3. Leave it as a seperate project that (but updated to the latest .net core)
  4. Wait till .Net Standard 2.0 comes out which is supposed to unify .Net full framework and .Net Core, I believe it is in preview

@thinkingserious
Copy link
Contributor

Hi @Gekctek,

I will take care of the build process, assembly signing, versioning and nuget package.

Please proceed with option 1. Thank you!

With Best Regards,

Elmer

@thinkingserious thinkingserious added difficulty: hard fix is hard in difficulty hacktoberfest labels Sep 30, 2017
@thinkingserious
Copy link
Contributor

Hi @Gekctek,

Do you still want to take this on? It's now hacktoberfest again :)

With Best Regards,

Elmer

@Gekctek
Copy link
Author

Gekctek commented Oct 15, 2017 via email

@thinkingserious
Copy link
Contributor

Thanks for the update @Gekctek,

I agree that the best route would be is to remove the dynamic functionality. This is what we have done in the official SDK, which no longer depends on this library.

With Best Regards,

Elmer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: hard fix is hard in difficulty type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants