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

chore: upgrade to latest poison and httpoison #41

Closed
wants to merge 1 commit into from

Conversation

0xcadams
Copy link

No description provided.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1335

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-34.4%) to 56.25%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/lob/client.ex 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
lib/lob/util.ex 1 90.91%
lib/lob/bank_account.ex 2 0%
lib/lob/client.ex 7 30.77%
Totals Coverage Status
Change from base Build 961: -34.4%
Covered Lines: 18
Relevant Lines: 32

💛 - Coveralls

@@ -53,7 +53,7 @@ defmodule Lob.Client do
end

def process_response_body(body) do
Parser.parse!(body, keys: :atoms)
Parser.parse!(body, %{keys: :atoms})
Copy link

Choose a reason for hiding this comment

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

It's more idiomatic to pass options as a keyword list rather than a map. I think I hit a snag in my recent updates of other Elixir libraries where I had to modify the format of the options from a map to the keyword list format.

Secondly, I think this would be cleaner if it called Poison.decode!/2 instead of Poison.Parser.parse!/2. Poison routes execution over to its parser module, so technically, the two calls are equivalent (for now), but that is an implementation detail that we don't necessarily want to rely on. Also, that would keep things a bit more flexible with respect to which specific JSON package in use (nowadays, Jason is more prevalent than Poison). E.g.

# Set up a named alias
alias Poison, as: Json

# ...


  def process_response_body(body) do
    Json.decode!(body, keys: :atoms)
  end

This way, if you ever decided to use Jason instead of Poison, you'd just need to make a single change at the top of the file (or you could even move this into configuration). But here you see the importance of using the keyword list instead of a map -- although Poison may tolerate (or tolerated?) options provided as maps, it's less likely that other packages would do the same.

Just food for thought!

@sudoku-lord
Copy link
Contributor

This should also be resolved in the latest PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants