Hi, sorry for adding to such a long thread

[1]

I do not like APIs that require string keys / other than camel case as input, such as the request-body using convertTo, for example

request_body = %{"convertTo" => "pdf", "data" => data}
Carbonex.create(finch_name, "path/to/template.odt", request_body)

should ideally be

Carbonex.create(template_path, data, convert_to: :pdf)

In general if you read Elixir source code a lot you will find that folks usually use camel case only, we do not do title case much, so it would be good to make it consistent.

Also exposing Finch as your HTTP API is probably not ideal (where does finch_name come from, etc) better to make it configurable if you have to but otherwise have a default API that does not require plugging this information.

Case in hand ExAws uses hackney but doesn’t ask you what pool, it just uses the :ex_aws pool.

[2]

README needs work, show me how to actually use the library and I don’t want to see stuff that is standard such as “Documentation can be generated with ExDoc and published on HexDocs” which is just part of the original boilerplate.

The README should describe what is available in the library and how to use it. If I copy and paste each chunk of code into iex it should execute, which it does not right now. Also it would seem that carbone.io would require an API key, but the way to configure it is unclear. This must be addressed.

I am not to impose a rigid structure, but it is my opinion that for any library, the README should include the following:

1. Name of the library
2. Succinct description of the problem it solves (do not write a nerd essay, keep it under 100 words)
3. If the library solves a common problem, explanation of the problem and analysis of all equivalent solutions, justification for using this library over others (this can be skipped if writing first-party platform SDKs, otherwise especially if the library is algorithmic or data-intensive, it would be good to add some benchmark results)
4. Installation & configuration (the “add this to mix.exs” section comes in here, also any environment variables & key configuration should be included, once I’ve read this section I should have everything needed to get the library integrated)
5. Some examples / usage (can be part of hexdocs instead)

And if you are nice then add changelogs, acknowledgements, reference, etc.

Link to hexdocs should be possible to find within the first 1 second of looking at the README.

[3]

For symmetry reasons, you might want to instead call the function Carbonex.render vs Carbonex.create the latter has no direct counterpart in the API of the service. In general when writing an Elixir SDK for a service you should match the name of whatever method/function published by the service.

So I would expect something like this

@type option :: {:convert_to, :pdf}
@type data :: binary() | map()
@spec render(template_name :: string(), data :: data(), options :: list(option())) ::
  {:ok, document_data :: binary()} | {:error, reason :: term()}

Note how we are typing the option as a 2-arity tuple, this is so the options can be passed in as a Keyword and its type will be checked. If you write it as a map it will be messier.

You can check out Stripe and how they intersperse different SDKs together alongside with cURL, the bottom line here is that integration engineers are very busy and you need to make sure your Elixir SDK is aligned with the service as much as possible.

[4]

carbonex/templates/Sample_template.odt

Wrong capitalisation, should be

carbonex/templates/sample_template.odt

Sweat the small stuff

[5]

See Carbonex requires CARBONE_TOKEN to be set in the environment, I would think that this is an Elixir anti-pattern and folks would be happier if you wrapped the configuration in a context object etc.

Read Library guidelines — Elixir v1.21.0-dev for a bit and again compare how ex_aws is configured… Will users want to use more than 1 token with your library? If no then you can keep what you have, or change it so you get it from Config.

Otherwise, you have to do something like this

defmodule Carbonex.Environment do
  @enforce_keys ~w(token)a
  defstruct token: nil
end

defmodule Carbonex do
  def render(environment, data, options) do
    …
  end
end

Which will be used thusly

environment = Carbonex.Environment{token: "…"}
Carbonex.render(environment, …)

Again use own judgement

[6]

Lastly from a maintainability point of view, does your platform publish OpenAPI v3 Specs? If so, consider code generation vs hand-rolling code

Nice review! One thing to note is that piping into a case has been described as a bad pattern. The suggested solution is to extract the cases into a function (with multiple heads).

Benefits:

1. Function name can be explanatory about what’s happening
2. Easier to test the ‘cases’

Hi, it’s a nice code for the first project. Here are the things I’ve noted during reading

Not important things

• mix format. Use spaces or tabs, not both

• Translate this

            response
            |> get_template_id_from_response
  

  to get_template_id_from_response(response)

• Do not refer to the functions in module like Carbonex.function, use just function.

• Translate

    case decode_json(response) do
      {:ok, map} -> map["data"]["templateId"]
      _ -> nil
    end
  

  to

    case decode_json(response) do
      {:ok, %{"data" => %{"templateId" => id}}} -> id
      _ -> nil
    end
  

  By the way, the code above occurs twice, so I’d suggest to DRY it.

• You can pipe into case, so instead

  result = Jason.encode(data)
  case result do
  

  you can write

  data
  |> Jason.encode()
  |> case do
  

Important things and design

• More tests. To test the HTTP interactions, you can use awesome Bypass library.

• Do not use System.get_env. I’d suggest just to create some structure which holds url, version and token for the carbone.io. This will make the code more pure (in functional sense) and easier to test

• Provide function which starts finch with given url, version and token.

• Learn specs. Proper and precise type annotations are better than any documentation.

• I think that get_document function should return the document, but not the HTTP response structure.

• create_multipart function checks the existence of a file, while it has already been checked in add_template function