-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
BufferedImageConverter cleanup #145
Comments
I'm happy to work on this. I could be useful to turn it into a project to submit 1 PR at the time. Can I do it or does it need to be someone with write access to the repo? |
Is that reasonable or is it better to just leave it as |
I've created a project: https://github.com/orgs/creativescala/projects/8/ Regarding the refactorings you describe above:
|
Consider the following:
BufferedImageConverter
is quite a mouthful. Can we shorten the name? Perhaps we use the term "Writer" for anything we can convert aPicture
into, and rename the currentWriter
toFileWriter
(andBase64
becomesBase64Writer
)?Can we make the
Frame
optional? Can we get it from aDefaultRenderer
? Maybe the idea of a default frame should be extracted into it's own effect (it's not really an effect, but it is associated with effects.)Do we need to return the value of type
A
when we convert to aBufferedImage
? Most of the time the user will throw it away. (Same issue exists forBase64
). Maybe the convention should be you get theA
if you convert to anIO
but we throw it away in you immediately run thatIO
. This would match what happens indraw
.Maybe what we need is a generic conversion effect type class. Something like
Java2dBufferedImageWriter
inJava2dWriter.scala
should be renamed and moved to its own file.Add tests.
Add documentation.
This issue could probably be usefully turned into a project. Each bullet could be it's own issue.
The text was updated successfully, but these errors were encountered: