Refactor Mix.Gleam#1
Conversation
- Renamed `Mix.Gleam.require!/0` to `Mix.Gleam.requirements!/0` as it has a
different meaning in Elixir.
- Added/ `@spec` annotations to public functions.
- Avoided making functions raise and return results (`{:ok, value}` |
`{:error, message}`)
- Now every function that raises ends with a ! (with the exception of
`Mix.Gleam.load_config/1` due to consistency with the Rebar compiler)
- Avoided raising from Elixir functions, and raise with `Mix.raise/1` almost
exclusively, raising with a descriptive message as opposed to raising with a
generic message and a stacktrace. The exceptions are the use of
`JSON.decode!/1` since there's no way to translate the error atoms to string
messages, as well as `File.cd!/2`.
- Simplify the calls to `Mix.raise/1` with the introduction oof a helper.
Now only 3 times the helper is called to return the value in the
`{:ok, value}` tuple, or to `Mix.raise` with a message.
- `Mix.Gleam.parse_dep!/2` now users pattern matching to build its specs.
- Renamed `@required_gleam_version` to `@gleam_version_requirement` for clarity.
- `Mix.Gleam.requirements!/0` raises with a descriptive message saying the
minimum gleam version isn't met, as opposed to before where if this
requirement is not met it wouldn't even raise
| with {:ok, output} <- | ||
| gleam(~W(export package-information --out /dev/stdout)), | ||
| json <- JSON.decode!(output), | ||
| {:ok, gleam_toml} <- Map.fetch(json, "gleam.toml") do |
There was a problem hiding this comment.
maybe?
{{:ok, gleam_toml}, :fetch_toml} <- {Map.fetch(json, "gleam.toml"), :fetch_toml} do {:error, :fetch_toml} ->
{:error, "\"gleam.toml\" key not found in \"gleam export package-information\" output"}There was a problem hiding this comment.
Initially i did that, but the only function that returns :error is Map.fetch/2 so I decided to simplify it.
| |> Enum.map(&parse_dep!/1) | ||
|
|
||
| dev_deps = | ||
| Map.get(json, "dev-dependencies", %{}) |
There was a problem hiding this comment.
Blocker imho:
The gleam.toml format is now consistent. The two sausage-case fields (dev-dependencies and tag-prefix) have been replaced by snake_case versions. Files using the old names will continue to work.
Source: https://github.com/gleam-lang/gleam/blob/v1.15.0-rc1/CHANGELOG.md#build-tool
... AFAIU we must support dev_dependencies and dev-dependencies (and merge both)
| {:error, | ||
| "The \"gleam\" executable is not available in your PATH. " <> | ||
| "Please install it, as one of your dependencies requires it"} | ||
| else |
There was a problem hiding this comment.
I have never seen def/catch/else syntax so far, unless I am blind it is also not mentioned here https://hexdocs.pm/elixir/try-catch-and-rescue.html But if it works, nvm.
There was a problem hiding this comment.
Neither have I but seems to work. Maybe it its worth documenting this
| @spec requirements!() :: :ok | ||
| def requirements!() do | ||
| case fetch_gleam_version() do | ||
| {:ok, gleam_version} -> |
There was a problem hiding this comment.
Do you still think a test is required, after the changes you made?
@eksperimental
There was a problem hiding this comment.
Well, in theory it should work, but so i theory it should have worked before the fix. Personally, since it's crucial I would test it.
|
LGTM except #1 (comment) which is not strictly related to this PR but to the original PR and changes in gleam tooling |
|
@eksperimental do you still have concerns/feedback on elixir-lang#14262? Specifically the PR is still marked as non-mergable/red to me because of:
Is the PR now fine for you (and could you remove the change request) or do you still see that there is something to do? |
Hi. There's no further requests on my side. I also don't see how to remove that. Maybe one of the change requests is still pending, something which I doubt |
|
Maybe https://github.com/elixir-lang/elixir/pull/14262/changes .. then |
3 participants
Renamed
Mix.Gleam.require!/0toMix.Gleam.requirements!/0as it has a different meaning in Elixir.Added/
@specannotations to public functions.Avoided making functions raise and return results (
{:ok, value}|{:error, message})Now every function that raises ends with a ! (with the exception of
Mix.Gleam.load_config/1due to consistency with the Rebar compiler)Avoided raising from Elixir functions, and raise with
Mix.raise/1almost exclusively, raising with a descriptive message as opposed to raising with a generic message and a stacktrace. The exceptions are the use ofJSON.decode!/1since there's no way to translate the error atoms to string messages, as well asFile.cd!/2.Simplify the calls to
Mix.raise/1with the introduction of a helper. Now only 3 times the helper is called to return the value in the{:ok, value}tuple, or toMix.raisewith a message.Mix.Gleam.parse_dep!/2now users pattern matching to build its specs.Renamed
@required_gleam_versionto@gleam_version_requirementfor clarity.Mix.Gleam.requirements!/0raises with a descriptive message saying the minimum gleam version isn't met, as opposed to before where if this requirement is not met it wouldn't even raise