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

[DRAFT] Elixir Optional parameters #113

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Doerge
Copy link

@Doerge Doerge commented Jun 13, 2024

This PR:

  • Moves optional positional parameters to a Keywordlist.
  • Documents optional parameters and their types, as best as possible.
  • Improves docs formatting.
  • Improves typespecs.
  • Only includes the beginning of the AWS docs for a method, and links (as best as possible) to the online page instead.
image

https://github.com/aws-beam/aws-elixir/blob/master/lib/aws/generated/s3.ex#L8374
Latest (16/7/2024):
https://github.com/Doerge/aws-elixir/blob/140deecfb1387b14bf5da4c683b4086b44bf9629/lib/aws/generated/s3.ex#L7655

TODO:

@Doerge
Copy link
Author

Doerge commented Jun 13, 2024

I'd still like to validate the optional parameters with Keyword.validate!, but because the optional params are popped off, and then the rest is passed down to the client, this is not possible. I.e. if an optional param is misspelled, it just get's sent to the client.

Possible solutions:
a. Group all client-related params under a keyword like client_opts.
b. Add a second optional positional parameter client_opts that is passed down to the client.

I think option a. is easier, and cleaner. Will draft that up.
Note: Tesla uses the adapter keyword.

@yordis
Copy link

yordis commented Jun 20, 2024

Since the operations already take a client argument, couldn't you just put the client opts there?

client
|> put_opts() # do things before
|> get_bucket_website() # only about operations

@Doerge
Copy link
Author

Doerge commented Jun 25, 2024

Ah, that's actually a really good idea! Completely bypasses the issue.
I'd probably add that as a little utility function:

defmodule AWS.Client do
  [...]
  def put_http_opts(%__MODULE__{} = client, opts) do
      {http_client, default_opts} = client.http_client
      final_opts = Keyword.merge(default_opts, opts)
    
      client
      |> AWS.Client.put_http_client({http_client, opts})
  end
end

my_client
|> AWS.Client.put_http_opts(foo: false)
|> get_bucket_website()

What do you think?

@yordis
Copy link

yordis commented Jun 26, 2024

That works; it is similar to https://github.com/elixir-tesla/tesla/blob/d39d575af3edb87cc156b803317ebb7f7670b45f/lib/tesla.ex#L182 So some extent, you are chasing something similar to Tesla but one layer above

@Doerge
Copy link
Author

Doerge commented Jul 16, 2024

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