-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add meaningful names to AnnotatedString annotations to make their usage understandable #55249
Comments
i want to work on this issue. |
I see the point here, and I think a |
Before 1.11 release is the only time to make such "breaking" changes (not really breaking because feature wasn't released). Otherwise Julia and all users are stuck with these decisions "forever".
Not sure what exactly you mean here... Now appears to be a common reasonable time to try using 1.11 in realistic scenarios, given that it's becoming more stable and regressions are getting fixed. In comparison, nightly is much less stable, so trying 1.12 features now and giving feedback based on actual experience is more challenging, only for the most dedicated users. |
Right, I should clarify that this is breaking with Julia v1.11-rc1, not a currently released Julia. However, my understanding of the alpha, beta, and rc stages is that non-bugfix changes are more appropriate for the alpha and maybe beta releases. On that note, with
I specifically mean this in the sense that this feature has been part of Julia 1.11 for a while now, and we're now just before the release of 1.11. In this sense "let's change an aspect of the API of a new 1.11 feature" is rather late in the development cycle. I do sympathise with only just trying a new feature now and having thoughts, but it places a fair bit of time pressure on me to make such changes and inconvenience other people involved in the release process if we decide this change should be part of 1.11. I think this could be a nice API tweak, it complicates construction but makes the intent in the values more obvious. Not 100% sold though, so I'd be interested in hearing other thoughts, and also on the matter of whether such a change is even potentially appropriate for an rc-stage release. |
@fatteneder I'm not sure quite what opinion you're trying to express via 👎, care to elaborate? |
So what if it's late in the cycle? |
I dislike that this is quality-of-life improvement might not happen, and that we might be stuck now with this until a hypothetical v2.
It would be nice to have this confirmed from someone in charge. IMO breaking RC should be ok, after all its the last chance to fix things before you are bound to the backwards compatibility guarantee that definitely applies to full release. |
I fully appreciate that, I just don't know how that jibes with the way that release candidates are handled (I don't think we have any docs anywhere on this?). It also makes annotations a bit more of a pain to write if you're writing a lot of them, but maybe that can be resolved with a constructor? |
The constructor can support both annotations formats: one convenient for manual writing (is it expected to be common?) and another convenient for any programmatic processing (and returned from Of course, if hand-writing annotations in code isn't supposed to be very common, then the corresponding format may not be needed at all. julia> NamedTuple{(:range,:label,:value)}[
(1:5, :x, 123),
(3:6, :y, 456),
# and more ...
]
2-element Vector{NamedTuple{(:range, :label, :value)}}:
(range = 1:5, label = :x, value = 123)
(range = 3:6, label = :y, value = 456) |
Why would this have to be a named tuple instead of a |
Mmm, so I'd say the contenders for the form are:
struct Annotation
range::UnitRange{Int}
label::Symbol
value::Any
end or similar. I do just wish this came up in #49586 rather than once it's part of a release candidate 🫠. |
IMO anything with reasonable names is better than opaque numbers, don't have a preference between 2 and 3.
With base Julia, one cannot try out a speicifc new feature – it's all or nothing. There are still known rough edges and regressions, but RC at least tend to have fewer of them, making it feasible to actually try using the upcoming version in real scenarios. I only noticed this issue when actually doing processing/transformations/aggregations of annotations in a string. |
Triage things this is something definitely worth doing. @tecosaur will try to do it within a fortnight and would be happy to receive help. |
To clarify on the release process issue, yes it is ok to break things in a release candidate in a new feature that is not yet in a final release. |
A good approach is to define |
I'm still looking for the time to make this change. In the meantime, I'll just ask if anyone has thoughts on what other functions that have the label-value pair without the range (e.g. Using |
I agree that |
Well, here we are, the last item on the 1.11 milestone. This is a good example of how Base is not a good venue for designing novel, complex APIs. Instead of having both AnnotatedString and julia 1.11, we have neither. |
I think more than anything else this illustrates the problem with:
As alluded to in my last message, and stated on Slack, over the past ~month I simply haven't had the time for this. The good news is that this busy period started winding down a few days ago, and so I can actually work on this now. |
Update: I think I have this implemented now, along with the required changes to StyledStrings and JuliaSyntaxHighlighting. I now need to do a few sanity checks and update the tests, then I'll put a PR up. |
But the whole point is that new APIs need time to try out and revise the design. That inherently makes it harder to work within Base. Is it workable to mark the new AnnotatedString APIs as experimental? I don't have a good handle on whether that would be realistic. Of course it wouldn't necessarily stop people from using it, but might be better than nothing. |
I was even a bit surprised this went into Base without trying out the interface in a package first... #49586 (comment) |
That approach worked quite well with the multithreading API, I think? |
It should be moved out of Base (into StyledStrings or its own stdlib). |
I think this highlights the danger in bringing in someone's more or less personal project into Base / stdlibs and all of a sudden it is on the critical path for releases. Especially, if that project is kind of new and "fun" and not old and boring. |
Anyway, I don't understand why this is on the milestone when the existing API would still work if using NamedTuples instead of tuples so adding that will not break anything. So I will take this off the milestone. |
From the linked PR
It's not just tuple -> named tuple, it's nested tuple-pair to flat named tuple. This changes |
Ok, then we declare the whole thing experimental. This has been on the milestone for two months for some new color highlighting thingy. Insanity. |
Removing from milestone since StyledStrings + AnnotatedString will be considered experimental |
This is IMO quite unintuitive and confusing to read:
Would be much nicer and self-explanatory to use meaningful names instead of numbers:
The text was updated successfully, but these errors were encountered: