Add test and followup for StyledStringsExt
#302
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #302 +/- ##
==========================================
- Coverage 84.31% 83.64% -0.68%
==========================================
Files 8 9 +1
Lines 778 746 -32
==========================================
- Hits 656 624 -32
Misses 122 122 ☔ View full report in Codecov by Sentry. |
kimikage
force-pushed
the
styledstrings_followup
branch
4 times, most recently
from
f56388f to
c64a1d2
Compare
kimikage
force-pushed
the
styledstrings_followup
branch
from
c64a1d2 to
cdd28e1
Compare
|
@tecosaur Is the following intended behavior? julia> StyledStrings.SimpleColor(RGB(1, 0, 0))
ERROR: MethodError: Cannot `convert` an object of type
RGB{FixedPointNumbers.N0f8} to an object of type
Union{@NamedTuple{r::UInt8, g::UInt8, b::UInt8}, Symbol}
The function `convert` exists, but no method is defined for this combination of argument types. |
| @@ -14,12 +14,13 @@ StyledStringsExt = "StyledStrings" | |||
|
|
|||
| [compat] | |||
| FixedPointNumbers = "0.5, 0.6, 0.7, 0.8" | |||
| StyledStrings = "1" | |||
| # StyledStrings = "1" # FIXME: workaround for unregistered StyledStrings | |||
There was a problem hiding this comment.
This should be fixed in the close future, but currently StyledStrings is unregistered and it should be v1 for the time being.
We are touching the internals of StyledStrings, so we run the risk of incompatibility. And that can't be prevented by the "1" cap anyway.
（In other words, if we follow the principle, we need to specify the patch version here.）
There was a problem hiding this comment.
We are touching the internals of StyledStrings, so we run the risk of incompatibility
Just FYI, SimpleColor is part of the public API in the 1.11 stdlib version, so the conversion implemented here shouldn't break in any 1.x version of StyledStrings.
There was a problem hiding this comment.
What constitutes a breaking change is ambiguous. If SimpleColor added a feature to have an alpha component, it would not break the code, but it would go against the user's intuition.
That is just an example. StyledStrings is community owned, although you are the lead developer. I don't believe anything bad will happen, but there is no guarantee how it will turn out.
For example, CI could cease to function effectively even though no one has done anything wrong.
| if isdefined(Base, :get_extension) | ||
| import Pkg | ||
| if VERSION < v"1.11" | ||
| Pkg.add(url="https://github.com/JuliaLang/StyledStrings.jl", rev="julia1-compat") |
There was a problem hiding this comment.
I don't know how long julia1-compat remains in effect, but it is more practical than main for now.
There was a problem hiding this comment.
I don't anticipate deleting the branch at any point, it will just become abandoned at some stage.
kimikage
changed the title
StyledStringsExtStyledStringsExt
I'd say so, given I didn't intend for the constructor to work directly with |
If you do not plan to make any changes in this package, I am ok with it. |
I do not understand the need for me to create this PR, but this PR itself might be necessary for sound development.