Fix Dialyzer warnings and remove ignore file #952
Conversation
In order to fix the dialyzer warnings, remove each one from the ignore file. When all warnings are fixed, the ignore file could be removed altogether.
Currently, the write_config_file/1 function in the installer checks the return value of a call to IO.binwrite/2. However, that function only ever returns :ok or raises an error when it fails to write. This patch removes the error handling, assuming the write works. Since the error handling is removed, the write will crash the installer if it fails, which is the intended bahavior.
Since removing all lines from Dialyzer's ignore file, the dialyzer.ignore-warnings file is no longer needed.
|
Did you see the conversation on Slack about the |
lib/mix/tasks/appsignal.install.ex
Outdated
| exit(:shutdown) | ||
| end | ||
|
|
||
| IO.binwrite(file, appsignal_config_file_contents(config)) |
There was a problem hiding this comment.
I think this change will fail for older Elixir versions.
See
There was a problem hiding this comment.
It won't fail, but it will silently swallow the error -- it needs to at least be :ok = IO.binwrite(...), for compatibility with older Elixir versions.
(but also, see the other comment, re: the error handling)
|
I don't think this is an error that will actually happen. If we want to add error handling anyway, we should wrap the call in a try block because it either returns an :ok or raises an error. It looks like the current implementation won't do what we think it does. |
|
The issue is that, in previous versions of Elixir ( |
Since Elixir 1.16, the IO.binwrite/2 function no longer returns an :ok- or :error tuple. Instead, it simply raises an error when it fails. To keep compatibility with previous versions of Elixir, this patch implements binwrite_with_result, which is a delegate of IO.binwrite/2 on versions before 1.16, and a try-catch wrapper for Elixir 1.16 and above. With that function in place, the precious implementation is restored, which prints "Failure!" before the error message in the unlikely event that the write fails.
|
Cool, then I propose this: 494a4d7 |
Co-authored-by: Tom de Bruijn <[email protected]>
Previously, the repository included a Dialyzer ignore file with a couple
of ignored warnings. This patch fixes all warnings and removes the file.
[skip changeset]