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:
- Name of the library
- Succinct description of the problem it solves (do not write a nerd essay, keep it under 100 words)
- 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)
- 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)
- 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.14.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
In any event, that’s my reasoning for not case-piping but again, it’s all a matter of taste!
