Return written image path from image.write#4
Conversation
094e101 to
fe81dd2
Compare
…format specifiers
fe81dd2 to
e4c32d5
Compare
|
This looks nice! Do you foresee wanting to suggest any other breaking changes in the near future? If so, should we release this now or wait to release a v2 with potentially more changes? I do not have anything I want to add at this point. If we merged this but didn't release it, you could still use it via a git dependency :) Also, strange about the tests. The package works when used regularly on your system with Elixir 1.18, right? |
|
I can't think of any other change right now, I'm just using it to generate resized versions of images at the moment. One thing I was wondering about, also regarding my change of the handling of the I didn't know it was possible to specify format options like that, as a |
Yes but I use none of the functions in those failing tests. I'll just paste the test output here for reference: Might also be some difference with the libvips version / OS or something like that. |
|
Yes, the string args are bypassing vix and going straight to vips. I can't remember the exact reason why I did it that way now, but I believe it was because I was either more comfortable with the vips API, or needed an arg that was not available to vix. Although this package sits on top of vix, I am inclined to support more of the vips API over the vix API, because vips is the real thing we are using. Using keyword args, would there be a better way of doing it in Gleam other than the |
|
I am running on Ubuntu 24.04, but I will try to upgrade my Elixir and see if I get the failing tests too. Thanks for the help triaging! |
|
I see, thanks for explaining, that sounds very reasonable to me, best to go direct to vips.
I actually kind of think it would be a decent option to drop the ImageFormat type altogether and just make the write / to_bitarray functions take a single string parameter. That would also solve my problem that this PR addresses, since the user supplies the final path in that case. I looked at some other bindings and the python ones have it like that (though they do also allow a keyword list) https://libvips.github.io/pyvips/vimage.html?highlight=write_to_file#pyvips.Image.write_to_file |
|
True! I will think about removing the ImageFormat type. In the meantime, what about also detecting when the user-supplied path ends in the image format extension, and then not adding it? Like if the user passes "output.png", then "output.png" will be written, not "output.png.png". |
|
Hmm not a fan of implicitly changing the behaviour like that tbh, I'd say either document the current behaviour (I added that in this PR too btw) or accept the full path only. |
|
Okay, sounds good! I like the changes in this PR. Could you address the one comment I left in the code? Then we can merge it. |
|
Hm, I don't see any comment, you may have forgotten to submit the review if you left a comment in review mode? |
|
Oh hahaha I did, thanks! |
|
Looks fantastic, thank you for the contribution! I will let this change sit a little bit, and push a v2 if no other change comes up in that period. Cheers! |
Closes #3
This change returns the path of the image as it was written to disk from the
image.writefunction.As I wanted just the path itself, without the format options appended to it, I had to separate those two out internally.
There are 2 breaking changes to the public API:
image.writereturns aResult(String, Snag)instead ofResult(Nil, Snag)Customvariant constructor now accepts separate parameters for extension and format options – I thought this made sense because I had to separate the two out internallyA side note: I get 11 failing tests (empty output) before any of my changes here locally, not sure what's up with that, may be that I'm on Elixir 1.18 instead of 1.17