Skip to content
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

create and save err if new image file is not successfully created #88

Merged

Conversation

MarkZsombor
Copy link
Contributor

What does this do?

Deals with Issue #65

Currently create/2 and save/2 fail silently if the file is not created as they don't check the result of the System.cmd call.

System.cmd returns a tuple with the result and the command exit status, so by pattern matching the result of cmd_mogrify and cmd_convert against a successful attempt ({_, 0}) these functions will now raise an error for unsuccessful exist statuses.

This is similar to the implementation for buffer in create/2.

How to test?

Try to save a file with an illegal file type, and error should be raised.

iex(3)> Mogrify.open("sample.png") |> format("jpg") |> save
%Mogrify.Image{
  animated: false,
  buffer: nil,
  dirty: %{},
  ext: ".jpg",
  format: "jpg",
  frame_count: 1,
  height: nil,
  operations: [],
  path: "/var/folders/hs/6c8zskxs7891h185psnyk3wm0000gn/T/18777-sample.jpg",
  width: nil
}
iex(4)> Mogrify.open("sample.png") |> format("doggos") |> save
** (MatchError) no match of right hand side value: {"mogrify: unable to open image 'doggos:': No such file or directory @ error/blob.c/OpenBlob/3537.\nmogrify: unrecognized image format `doggos' @ error/mogrify.c/MogrifyImageCommand/5062.\n", 1}
    (mogrify 0.7.4) lib/mogrify.ex:30: Mogrify.save/2

What needs to be considered ?

It may be nice to have it throw a better error, but I'd leave the shape of that open for discussion

@talklittle
Copy link
Collaborator

Thanks. Agree that better error would be nice as a future improvement. Any error is better than silent failure so merging now.

@talklittle talklittle merged commit f72e708 into elixir-mogrify:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants