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 request helper to reduce duplication. #206

Merged
merged 1 commit into from
Feb 7, 2017

Conversation

asummers
Copy link
Contributor

@asummers asummers commented Feb 3, 2017

Coverage went down somehow, though this is the same code, more or less. This also suggests an obvious API change that I would like to propose -- making body be the first argument of Stripe.Request. In the case of e.g. update, it would look like:

  @spec update(String.t, map, map, list, module, Keyword.t) :: {:ok, struct} | {:error, Stripe.api_error_struct}
  def update(endpoint, changes, schema, nullable_keys, module, opts) do
    changes
    |> Changeset.cast(schema, :update, nullable_keys)
    |> Stripe.request(:post, endpoint, %{}, opts)
    |> handle_result(module)
  end

I have this done locally and it looks pretty clean throughout. I can push to this branch if you all want, or a new one. This module would look like: https://gist.github.com/asummers/49384f353da45752b0cbb3ed61cbdc01 with handle_result being public because it is used in Stripe.Card.create/4.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 26.027% when pulling 87fc391 on asummers:add-request-helper into fab5b57 on code-corps:2.0.

@joshsmith
Copy link
Contributor

@DavidAntaramian could use your eyes on this but I like the proposal above re: changing to have body be first arg.

@joshsmith joshsmith merged commit f2d88da into beam-community:2.0 Feb 7, 2017
@joshsmith
Copy link
Contributor

🙌

@asummers asummers deleted the add-request-helper branch February 7, 2017 02:29
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.

3 participants