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 --head, --form, --user and --location flags to parser #5

Merged
merged 10 commits into from
Jun 6, 2024

Conversation

kevinschweikert
Copy link
Contributor

I've added some more flags to parse from the curl command. This should cover most use cases. Only missing thing could be to support the @ syntax in, for example, curl [URL] -d @filename.txt to load from a file. We could read the file in Elixir, but I don't know if we should support it.

I didn't yet do the other direction in to_curl in this PR for the new flags. Should i add them here or could this be a seperate PR?

@derekkraan
Copy link
Owner

I would be happy with a note in the documentation that we don't implement the read file syntax. If someone needs it, they can build it and contribute back a PR.

Re: to_curl implementations of these options. You can either add in this PR or add a TODO at the bottom of the README to cover it.

If you make those doc changes then I can merge this one.

Thanks for all your contributions btw!

@derekkraan
Copy link
Owner

Could you also add an entry to the CHANGELOG.md?

@kevinschweikert
Copy link
Contributor Author

Thanks! I will update the PR tonight

@kevinschweikert
Copy link
Contributor Author

Hey @derekkraan i've extended the PR with your suggestions and took some time to clean up the documentation and test files. Hope that's okay for you!

@kevinschweikert kevinschweikert force-pushed the feature/more-flags branch 2 times, most recently from f4cbb27 to 47979aa Compare June 6, 2024 06:43
Copy link
Owner

@derekkraan derekkraan left a comment

Choose a reason for hiding this comment

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

This is looking very nice! I think the auth bit is a little tricky, and I had a couple other minor comments as well that I'd like to see addressed before I merge.

lib/curl_req.ex Outdated Show resolved Hide resolved
lib/curl_req.ex Show resolved Hide resolved
lib/curl_req.ex Outdated Show resolved Hide resolved
lib/curl_req.ex Outdated
def from_curl(curl_command), do: CurlReq.Macro.parse(curl_command)

@doc """
Same as `from_curl/1` but as a sigil. The benefit here is, that you don't need to escape the string
Copy link
Owner

Choose a reason for hiding this comment

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

Also, the parsing and conversion to a %Req.Request{} is done at compile-time.

README.md Outdated Show resolved Hide resolved
@kevinschweikert
Copy link
Contributor Author

@derekkraan PR should be ready now ;)

@derekkraan derekkraan merged commit 724bcdc into derekkraan:main Jun 6, 2024
6 checks passed
@derekkraan
Copy link
Owner

Appreciate all the effort you're putting in here!

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