Group4Layers / ex_image_info

ExImageInfo is an Elixir library to parse images (binaries) and get the dimensions (size), detected mime-type and overall validity for a set of image formats. It is the fastest and supports multiple formats.
https://hex.pm/packages/ex_image_info
Other
95 stars 4 forks source link

Make `seems?/2` always return a boolean #4

Closed warmwaffles closed 9 months ago

warmwaffles commented 9 months ago

Predicate methods should always return a boolean. Mixing true | false | nil is a bit inconsistent.

rNoz commented 9 months ago

Hey @warmwaffles thanks for this.

While I understand the criteria, the idea of returning nil was some sort of "unsupported and/or not recognized format", in case other formats are provided, while false indicates purely that the binary it is not that image format.

However, if no objections, I'll accept it in a couple of days ;)

warmwaffles commented 9 months ago

Heh, the docs don't specify that nil means it is unsupported though. Just says "false otherwise".

https://github.com/Group4Layers/ex_image_info/blob/e9627da5990413c291fb66134adf40ac330f9e34/lib/ex_image_info.ex#L82-L86

Not a huge deal, just didn't realize that it was intentional. My use case is just checking if the image file type is supported in our application. Which we would coerce the nil to false using ==

@spec supported(String.t()) :: boolean()
def supported?(path) do
  case File.read(path) do
    {:ok, content} -> ExImageInfo.seems?(content) == true
    _ -> false
  end
end
rNoz commented 9 months ago

Sorry, in that case I missed that in the docs. Do you mind we keep the original behavior? In that case, maybe you can update the docs in this PR ;)

warmwaffles commented 9 months ago

Yea I can open another PR with the docs rectified. There are other places where I can see nil is being used.