Update API and add conditional dependencies #45
Conversation
and remove old commented-out code I committed by mistake
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
==========================================
+ Coverage 89.34% 91.21% +1.87%
==========================================
Files 11 14 +3
Lines 169 205 +36
==========================================
+ Hits 151 187 +36
Misses 18 18
Continue to review full report at Codecov.
|
| struct SeparateSpace{A<:AbstractDither} <: AbstractFixedColorDither | ||
| alg::A | ||
| end |
There was a problem hiding this comment.
Deleting a method without any deprecation can be very very breaking and thus triggers a major version v2.0.0.
Usually, a better practice is to start with some deprecations first to accumulate more breaking changes so that we don't break people's code too often. Might just keep them here for a while or move to a new file src/deprecated.jl and insert a deprecation message. For example, if I understand this change correctly:
struct SeparateSpace{A<:AbstractDither} <: AbstractFixedColorDither
alg::A
function SeparateSpace(alg::AbstractDither)
Base.depwarn("SeparateSpace is no longer need, please load `Clustering` package instead.", :SeparateSpace)
end
endThere was a problem hiding this comment.
I was actually thinking about making this v2.0.0 as it breaks some previous assumptions:
Previously, to apply per-channel dithering, methods had to be wrapped in the SeparateSpace() meta-method. In this version, we apply channel-wise dithering by default, such that
dither(Gray.(img), FloydSteinberg()) # => binary dithering (black & white output)
dither(RGB.(img), FloydSteinberg()) # => per-channel binary dithering (2^3 color output)So this release is already going to be breaking w.r.t. the outputs!
Would you be ok with dropping SeparateSpace if we make this v2.0.0?
There was a problem hiding this comment.
Okay, this sounds like a valid argument to me.
Just a quick question: does this dither(RGB.(img), FloydSteinberg()) require loading a Clustering? If so then we might want to make it a direct dependency.
There was a problem hiding this comment.
No, that's going to be the default behaviour. (Clustering.jl just adds optional automatic color scheme generation.)
I have one last question: is there a best practice for doc strings on functionality added by conditional dependencies? Is it possible to just add the following to src/clustering.jl?
"""
[Extra docstring]
"""
dither
"""
[Extra docstring]
"""
dither!My thinking is that these docstrings should also only be loaded when using Clustering.jl.
There was a problem hiding this comment.
This is an interesting question. You can actually have more than one piece of docstrings for the same function:
"""
foo(x)
The docstring for foo(x)
"""
foo(x)
"""
foo(x, y)
The docstring for foo(x, y)
"""
foo(x, y)and one can check the docstring for the specific method:
help?> foo(1)
foo(x)
The docstring for foo(x)
help?> foo(1, 2)
foo(x, y)
The docstring for foo(x, y)without the arguments, it prints all docstring:
help?> foo
search: floor pointer_from_objref OverflowError RoundFromZero unsafe_copyto! functionloc
Couldn't find foo
Perhaps you meant for, floor, fdio, fd, fld, fma, eof, cos, cot, log, mod, xor, Bool, do or foldl
foo(x)
The docstring for foo(x)
────────────────────────────────────────────────────────────────────────────────────────────────
foo(x, y)
The docstring for foo(x, y)
|
For small projects, none of these are required but still I'd like to write it down in case you're not aware of: It would be great to keep track of noteworthy breaking changes in README.md or CHANGELOG.md. See also https://github.com/JuliaGraphics/ColorVectorSpace.jl#abs-and-abs2 and https://github.com/JuliaImages/ImageTransformations.jl/blob/master/CHANGELOG.md. An alternative is to rebase these 19 commits into a few (say, 3-5) commits and then directly merge without squashing. This would give a more clear changelog in git history form; again another case of why we want to make each PR logically small 😆. |
I will add that!
Yeah, I should have broken issue #39 down into three separate "atomic" issues. That's how I ended up with this large PR. Next time I'll plan more carefully. |
Closes #39:
API updates:
SeparateSpace(andAbstractFixedColorDither) and instead automatically applies per-channel dithering on images with multiple channelsNew functionality using Requires.jl
support for ColorSchemes.jl
support for braille printouts using UnicodePlots.jl:
support for Clustering.jl for automated color palette generation