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

Publish as Curl.Parser.Net and Curl.Converter.Net as NuGet packages #99

Merged
merged 7 commits into from
Oct 9, 2022
Merged

Publish as Curl.Parser.Net and Curl.Converter.Net as NuGet packages #99

merged 7 commits into from
Oct 9, 2022

Conversation

jiahao1553
Copy link
Contributor

Sorry for the delay in making the code changes you mentioned in #63.
Please review the request. Thank you.

@jiahao1553
Copy link
Contributor Author

@olsh Please have a look. Thanks.

@@ -0,0 +1,52 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think that the package is not useful per se. I'd suggest merging the code into Curl.Converter.Net project.
Let me know what do you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't really get what you mean. Do you mean merging Curl.Parser.Net to Curl.Converter.Net?

Copy link
Owner

Choose a reason for hiding this comment

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

Do you mean merging Curl.Parser.Net to Curl.Converter.Net?

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olsh I think it can be separated because the output from the Curl.Parser.Net can be used to generate for example JS code based on the project need. In this case, they don't have to include the Curl.Converter.Net project. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Good point, agree. Then probably we should give more precise names for the libraries?
How about:
Curl.CommandLine.Parser
Curl.HttpClient.Converter
?

In this case we can implement another converters in the future like:
Curl.Flurl.Converter
Curl.RestSharp.Converter

@olsh
Copy link
Owner

olsh commented Sep 21, 2022

Hi @jiahao1553,

Thank you for a great job! 🔝
Could you please take a look at the suggestions?

@jiahao1553
Copy link
Contributor Author

@olsh I updated the code based on your comments. Let me know if there is anything else you need me to update 😊

@olsh
Copy link
Owner

olsh commented Oct 1, 2022

Hi @jiahao1553
Thank you for the update 👍🏻
Could you please take a look a the suggestions?

@jiahao1553
Copy link
Contributor Author

@olsh please take a look at my reply. Thanks

@olsh
Copy link
Owner

olsh commented Oct 5, 2022

@jiahao1553 which one?
I think I've replyed in all our threads.

@jiahao1553
Copy link
Contributor Author

@olsh this is the reply I meant. Hope it answer your question.

#99 (comment)

@@ -0,0 +1,52 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Owner

Choose a reason for hiding this comment

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

Good point, agree. Then probably we should give more precise names for the libraries?
How about:
Curl.CommandLine.Parser
Curl.HttpClient.Converter
?

In this case we can implement another converters in the future like:
Curl.Flurl.Converter
Curl.RestSharp.Converter

@olsh
Copy link
Owner

olsh commented Oct 6, 2022

@olsh this is the reply I meant. Hope it answer your question.

#99 (comment)

Ouch, my bad, I forgot to hit the request changes button.

@jiahao1553 jiahao1553 requested a review from olsh October 8, 2022 06:07
@jiahao1553
Copy link
Contributor Author

@olsh changes are done. Please take a look. Thanks 😊

Copy link
Owner

@olsh olsh left a comment

Choose a reason for hiding this comment

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

@jiahao1553 could you please take a look at the suggestions?

@jiahao1553
Copy link
Contributor Author

Replied and updated the code. Please take a look @olsh

@jiahao1553 jiahao1553 requested a review from olsh October 8, 2022 16:03
@olsh
Copy link
Owner

olsh commented Oct 8, 2022

@jiahao1553 let's drop .netstandard and try to keep the code as it is for now.
#99 (comment)
Also, could you please merge master branch to your branch to resolve conflicts?

@jiahao1553
Copy link
Contributor Author

@olsh I have completed the changes. Please take a look. Thanks.

@olsh olsh merged commit 2310728 into olsh:master Oct 9, 2022
@olsh
Copy link
Owner

olsh commented Oct 9, 2022

Thank you for the contribution

@jiahao1553
Copy link
Contributor Author

You're welcome @olsh :)

@olsh
Copy link
Owner

olsh commented Oct 15, 2022

@jiahao1553
I published the packages, thank you! 👍🏻
https://www.nuget.org/packages/Curl.CommandLine.Parser/
https://www.nuget.org/packages/Curl.HttpClient.Converter/

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.

2 participants