-
Notifications
You must be signed in to change notification settings - Fork 23
Handle Unknown errors in Erlang #55
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
Handle Unknown errors in Erlang #55
Conversation
src/simplifile.gleam
Outdated
| /// "sticky bit" on a regular file (not a directory). | ||
| Eftype | ||
| /// Host is down. | ||
| Ehostdown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. Would it be better to use Unknown?
src/simplifile_erl.erl
Outdated
| end. | ||
|
|
||
| error_reason_to_binary(Reason) -> | ||
| unicode:characters_to_binary(io_lib:format("~tp", [Reason])). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think atom_to_binary would be better used here.
Would it also make sense to uppercase the string? To match how it is written in POSIX (and also also the JavaScript target)
test/create_directory_bad_arg.mjs
Outdated
|
|
||
| export function createDirectoryWithBadArg(arg) { | ||
| return createDirectory(arg); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module could be removed and the @external reference ./simplifile_js.mjs directly.
|
@felixakiragreen thank you for fixing this! I think the main issue here is the crash/difference in error behavior. I think first, a PR fixing the erlang ffi to make use of the Unknown error variant to prevent the crash would be good that could be released as a patch version. If we decide to change the list of recognized errors, we can do that in a follow up PR. |
| {error, Reason} | ||
| {error, Reason}; | ||
| {error, Reason} -> | ||
| {error, {unknown, string:uppercase(atom_to_binary(Reason))}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The atom_to_binary function defaults to utf8, so it should be fine to pass the resulting binary to string:uppercase
| @@ -1,9 +1,10 @@ | |||
| # Changelog | |||
|
|
|||
| - Add documentation for 'mode' field in FileInfo. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a docs update that doesn't affect the version number, so I think it shouldn't be in the changelog.
|
closes #54 |
Ehostdown & handle Unknown errors in Erlang|
Thank you so much! 🙏 |
Issue Described Here
How this happened:
{error,ehostdown}when trying to write.append()to a file mounted via smb.What I did:
is_posix_errorinsimplifile_erl.erl.Unknown, but on Erlang it crashes.unknownerror case toposix_result, which converts the reason into a string.Unknowntype.Questions: