{"action":"created","issue":{"url":"https://api.github.com/repos/CargoSense/ex_aws/issues/137","repository_url":"https://api.github.com/repos/CargoSense/ex_aws","labels_url":"https://api.github.com/repos/CargoSense/ex_aws/issues/137/labels{/name}","comments_url":"https://api.github.com/repos/CargoSense/ex_aws/issues/137/comments","events_url":"https://api.github.com/repos/CargoSense/ex_aws/issues/137/events","html_url":"https://github.com/CargoSense/ex_aws/issues/137","id":146388869,"number":137,"title":"Proposal: Pure / Impure breakup","user":{"login":"benwilson512","id":247817,"avatar_url":"https://avatars.githubusercontent.com/u/247817?v=3","gravatar_id":"","url":"https://api.github.com/users/benwilson512","html_url":"https://github.com/benwilson512","followers_url":"https://api.github.com/users/benwilson512/followers","following_url":"https://api.github.com/users/benwilson512/following{/other_user}","gists_url":"https://api.github.com/users/benwilson512/gists{/gist_id}","starred_url":"https://api.github.com/users/benwilson512/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/benwilson512/subscriptions","organizations_url":"https://api.github.com/users/benwilson512/orgs","repos_url":"https://api.github.com/users/benwilson512/repos","events_url":"https://api.github.com/users/benwilson512/events{/privacy}","received_events_url":"https://api.github.com/users/benwilson512/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2016-04-06T18:00:19Z","updated_at":"2016-04-16T16:18:09Z","closed_at":null,"body":"Right now we have a problem. There are 2 orthogonal concerns that effect every function. For every function we want one version that returns an `{:ok, result} | {:error, reason}` tuple, and one version denoted with a `!` that raises on error and otherwise just returns the result. Also for every function we want the option to use a default configuration, and the option to specify a particular configuration.\r\n\r\nThis means that every function gets basically 4 variants. Using `S3.list_objects` as an example we have\r\n\r\n```elixir\r\nS3.list_objects(\"my-bucket\") #=> {:ok, []}\r\nS3.list_objects!(\"my-bucket\") #=> []\r\nS3.Impl.list_objects(config_overrides, \"my-bucket\") #=> {:ok, []}\r\nS3.Impl.list_objects!(config_overrides, \"my-bucket\") #=> []\r\n```\r\n\r\nIn addition to being just kinda sucky, these functions I think reflect a deeper problem. There's really 2 things going on here. There's the request building logic that maps a input into an AWS request, and then there are 4 modes of making that request. The current approach conflates these, and so we are forced to do these 4 modes for every request building function.\r\n\r\nMy current client approach basically tries to create an object out of a module. It ties a bunch of data (the configuration on that module) to a bunch of behaviour.\r\n\r\nHere's the proposed alternative:\r\n```elixir\r\nS3.list_objects(\"my-bucket\") |> ExAws.request #=> {:ok, []}\r\nS3.list_objects(\"my-bucket\") |> ExAws.request! #=> []\r\nS3.list_objects(\"my-bucket\") |> ExAws.request(config) #=> {:ok, []}\r\nS3.list_objects(\"my-bucket\") |> ExAws.request!(config) #=> []\r\n```\r\n\r\nI'm splitting up the pure mapping of input to some request struct from the actual impure mechanics of executing that a request on that struct. This will dramatically simplify the code. I only need one function to generate the data, and then 4 functions can serve to enable the 4 modes of making a request for EVERY request data struct.\r\n\r\nDownsides: This introduces an extra function call that people need to add anywhere they currently have just one function call.\r\n\r\nUpsides: By separating the pure from the impure it makes it wildly easier to use in all of the other use cases.\r\n\r\n- No need to call the `Impl` modules in order to pass custom config in.\r\n- You can put a function between the `list_objects` call and the `request` call to do any transformation of the data inside. This right now requires creating a whole new client module and overriding the `request` function.\r\n- Gone is all the silly meta-programming required to generate the 4 versions of every function.\r\n- It's a lot easier to see how we might batch requests, because an ordinary call like `list_objects` is just a datastructure. We can combine those together inside a Batch structure and then execute that.\r\n\r\nThoughts? @bruce @ericmj @adamkittelson"},"comment":{"url":"https://api.github.com/repos/CargoSense/ex_aws/issues/comments/210848852","html_url":"https://github.com/CargoSense/ex_aws/issues/137#issuecomment-210848852","issue_url":"https://api.github.com/repos/CargoSense/ex_aws/issues/137","id":210848852,"user":{"login":"benwilson512","id":247817,"avatar_url":"https://avatars.githubusercontent.com/u/247817?v=3","gravatar_id":"","url":"https://api.github.com/users/benwilson512","html_url":"https://github.com/benwilson512","followers_url":"https://api.github.com/users/benwilson512/followers","following_url":"https://api.github.com/users/benwilson512/following{/other_user}","gists_url":"https://api.github.com/users/benwilson512/gists{/gist_id}","starred_url":"https://api.github.com/users/benwilson512/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/benwilson512/subscriptions","organizations_url":"https://api.github.com/users/benwilson512/orgs","repos_url":"https://api.github.com/users/benwilson512/repos","events_url":"https://api.github.com/users/benwilson512/events{/privacy}","received_events_url":"https://api.github.com/users/benwilson512/received_events","type":"User","site_admin":false},"created_at":"2016-04-16T16:18:09Z","updated_at":"2016-04-16T16:18:09Z","body":"Thanks for your feedback.\r\n\r\n## Configuration\r\n\r\nThere are a few cases of configuration we need to worry about. The first is general configuration that is already placed on `:ex_aws` itself. This will naturally be what a bare `ExAws.request` uses. There is also request that is placed under a given service like\r\n\r\n```\r\nconfig :ex_aws, :s3,\r\n  region: \"us-west-1\"\r\n```\r\n\r\nThis too will simply be used by the default `ExAws`.\r\n\r\nFor people who had made custom modules that `use ExAws.S3.Client` or similar, the migration path is fairly easy there too. If they made a `MyS3` they just basically do\r\n\r\n```\r\ndefmodule MyApp.MyS3 do\r\n  def config_root, do: #whatever they had as their previous custom config\r\n\r\n  def request(operation, config \\\\ config_root()) do\r\n    ExAws.request(operation, config)\r\n  end\r\nend\r\n```\r\n\r\n## Streaming\r\n\r\nThis is a bit of a tougher question. Conceptually there are basically 3 parts to a streaming function. We'll use `list_objects` from S3 as the example. First you need the ordinary `list_objects` call. Then you need the configuration, and finally you need code which bundles those two together inside of an enumerable. Because the exact method of pagination varies by service and even by operation, I highly doubt that the enumerable bundling code can be generic.\r\n\r\nFortunately, we can still provide a pretty generic interface I think. Let me know what you think of this.\r\n\r\n```elixir\r\n\"my-bucket\"\r\n|> S3.list_objects\r\n|> ExAws.stream!\r\n```\r\n\r\nAs part of the datastructure we return from an ordinary `S3.list_objects` function call we include something a key like `%{stream_builder: &anon_function/2}`. This anonymous function takes the operation struct as an argument, and also a config as an argument, and returns an enumerable.\r\n\r\nThe `ExAws.stream!` function is basically just\r\n```elixir\r\ndef stream!(op, config \\\\ default_config())\r\ndef stream!(%{stream_builder: nil}, config), do: raise ArgumentError, \"Operation #{foo} is not configured for streaming\"\r\ndef stream!(%{stream_builder: fun} = op, config) do\r\n  fun.(op, config)\r\nend\r\n```\r\n\r\nI rather like this approach. Once again it keeps a nice separation of concerns between building the data required for an operation and how we want to consume the result. It also means we don't have yet another axis in our table up there for `stream` vs non stream functions.\r\n\r\nThoughts?"}}