More consistent return value for annotations #53281
Conversation
There are two different kinds of return value that annotations may
currently produce.
1. A vector of annotations (Pair{Symbol, Any})
2. A vector of region-annotation tuples (Tuple{UnitRange{Int},
Pair{Symbol, Any}})
Currently, there is an excess of argument-return-type combinations:
1. (string) -> vector of region-annotation tuples
2. (substring, region) -> vector of annotation values
3. (string, region/index) -> vector of annotation values
4. (char) -> vector of annotation values
While wanting to get the annotations of a substring, it became apparent
that it behaved differently to fetching the annotations of an
AnnotatedString{String}. This is undesirable. Upon further reflection,
the diversity of return types when calling annotations on a string-like
form is excessive, and behaviours 1-3 can be merged into a single
behaviour, resulting in a simpler set of combinations:
1. (string-like, region/index?) -> vector of region-annotation tuples
2. (char) -> vector of annotation values
I consider this simplification highly desirable.
Since this changes the API, the usage in StyledStrings is also updated
to match, and we bump the version of StyledStrings to get the new
version compatible with this revised API.
|
Changes included in the StyledStrings bump: |
|
Just FYI, I lost your good commit message here because github now automatically replaces them with the text in the PR title. If you want to ensure that content is preserved, I recommend including it in the Github PR text. |
|
It looks like Example.jl is hitting an abort signal on all CI with this commit included. Is it possible you put |
|
I don't think so, and it's hard for me to see how this commit would affect anything with |
|
I have just noticed that the implementation of |
Reverts #53281 as it appears to have broken CI on all subsequent commits: #53281 (comment) According to the CI logs, it is this test that aborts early on in the rr-trace log: ``` Pkg.test(TEST_PKG.name; coverage = coverage_path) ```
|
Are there any |
|
Aha! I think I might have found it, with the I'm not sure how to test this theory though... |
|
If you push a new PR, the net tests will pass/fail based on whether that fixes it. If necessarily, you might just delete the |
|
#53333 now exists (hey, that's a surprisingly memorable PR number), which should let us confirm (or disprove) my suspicion 🤞 |
Relands #53281 with some fixes noticed, though not the original causes of the failure.
Relands JuliaLang#53281 with some fixes noticed, though not the original causes of the failure.
Relands JuliaLang#53281 with some fixes noticed, though not the original causes of the failure.
This is a generally nice simplification, that tweaks the
annotationsAPI.It also requires the adjustment in
JuliaLang/StyledStrings.jl#38 to be made, hence the
bump.