-
Notifications
You must be signed in to change notification settings - Fork 3
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 more auths and fine tuned run_steps #14
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the PR! I am in general in favour of expanding the capabilities, but this PR needs some polish before I can merge. In particular, I'm not clear on how :exclude
and :only
will be used, since they are not used anywhere and there are no tests covering their use.
@@ -88,8 +104,9 @@ defmodule CurlReq do | |||
headers = | |||
req.headers | |||
|> Enum.reject(fn {key, _val} -> key == "cookie" end) | |||
|> Enum.flat_map(fn {key, value} -> | |||
[header_flag(flag_style), "#{key}: #{value}"] | |||
|> Enum.flat_map(fn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you undo the formatting change here?
@@ -70,14 +89,11 @@ defmodule CurlReq do | |||
""" | |||
@spec to_curl(Req.Request.t(), Keyword.t()) :: String.t() | |||
def to_curl(req, options \\ []) do | |||
flag_style = Keyword.get(options, :flags, :short) | |||
opts = Keyword.validate!(options, flags: :short, run_steps: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
_ when is_boolean(option) -> | ||
option | ||
|
||
[{:except, excludes}] -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for this, so that it becomes clearer what the purpose is of this addition? (same applies for :only
).
req.request_steps | ||
|> Enum.filter(fn {step_name, _} -> | ||
case option do | ||
_ when is_boolean(option) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is a little convoluted in my opinion. I'd like to at least see a pattern match on false
to remove that branch from this path.
def run_steps(req, false), do: req
Please also include an entry in Changelog.md describing the changes. |
Add support for more auth schemes into their Curl counterparts (eg
-n
for:netrc
).This requires that the
auth
step fromReq
doesn't run, since it actually validates the netrc file and inserts the basic auth from it. It also will base64 encode Basic auth, and cause there to be duplicates between the headers and auth fields of theReq
struct. So I added options:except
and:only
to therun_steps
function to have more fine-tuned control over which steps are run.