-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use parse tree #2
Conversation
Beautiful :-) The highlighting of It makes me want to include tools - maybe even a parser mode - to further process the content of docstrings and comments within JuliaSyntax. But I know I shouldn't go there 😆 |
Thanks Claire 🙂
It's inspired by https://code.tecosaur.net/tec/simple-comment-markup, which I consistently find slightly nice to have.
Heh, yea, I can see the temptation. I'm tempted to add an ad-hoc markdown parser for docstrings. For the future, something else I'm considering is making an extensible list of highlighting passes be used. That way DSLs introduced by macros could get custom highlighting, for instance (I'd love to see Once I've applied the tweak we discussed on slack, I'm thinking we can just merge this and improve from here. The particulars of the highlighting scheme (the faces used, and where they're applied) are explicitly mentioned as subject to change. |
One point that might be an improvement: In the help, example |
@jakobnissen two things
|
99.9% will use defaults and will now be confused about what came from markdown rendering and what came from the actual REPL. Making the default non-confusing seems like a good idea. |
Thanks for the feedback @BioTurboNick! To keep the conversation rolling, I'll lay out my initial thoughts on your comments.
[behaviour] Matching parent colouring to the context seems viable, but a bit of a hassle. I'm inclined to bump that off to a future PR (possibly by someone else) even if we're sold on it. [defaults] There is also a no-colour option available by setting
[defaults] I think this is a question of defaults, since I don't mind discussion on defaults, but I would like to separate it out from discussion on the capabilities/design 🙂.
[defaults] Maybe? I'm not entirely sure what defaults
Currently, they are? I'm assuming by literals you mean
[defaults] That's an interesting thought...
[defaults] perhaps, I think this might be somewhat divisive though, IIRC a few people on slack liked it different [behaviour] refer to earlier comments on parent highlighting matching their context
This behaviour was suggested by Fredrik Ekre: https://fredrikekre.se/posts/highlight-julia/#infix_operators_and_assignment Maybe it's worth making an option? Not sure.
Hmm, I'm more open to making all commas grey than matching the parenthesis level from an implementation difficulty perspective, but both ideas seem worth considering aesthetically to me.
I think semantically
|
@tecosaur - To be fair you did direct me to this PR for feedback :-) I can break out the defaults into a separate issue if you like. The drive behind my suggestions, is that 1) a color change should signal something important, and too many color changes in a small space interferes with intelligibility; and 2) minimize the number of times the same color can mean different things. A default that follows these principles more closely will be a nicer user experience, however you want to achieve that. Just a few other response points:
|
All good, I just think it's helpful to separate the two in conversation. If nothing else, one takes a lot more effort than the other 😛 We can also continue this conversation after this PR is moved, I like to merge changes that improve a situation even if they're not perfect/end-state by themself, but that doesn't mean I think the work/conversation should stop once this PR is merged. I'm in agreement with regards to wanting colour changes to signal semantic information/be actively helpful not just mimic a rainbow. The complication I think is that there are multiple reasonable ways of doing so. Figuring out a good balance here is an active WIP as far as I'm concerned One of the reasons why more faces are used than there are ANSI colours, because I want to allow going beyond ANSI 4-bit colour. I think this affects "minimize the number of times the same color can mean different things" a bit. I think we can "have our cake at eat it too" (somewhat) by using the inheritance pattern. E.g. the faces
I think you may have mixed up the bright and non-bright colours? Anyway, we'd also need to work out how to enact this change (we've got at least two options). |
Let's merge this since it's an improvement, but keep on talking about how it can be made better 😀. @BioTurboNick if I could convince you to make two issues, one on changes to defaults you think would be a good idea, and the other on changes in behavior that would be ideal: if not, no worries 🙂. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable, though I think there's some lack of clarity in the code regarding tokens vs interior nodes. I generally find it pays to split those cases apart based on haschildren()
.
(I've thought before that it could be useful to have the kind()
of a for
token be different from the kind of the for
interior node. This would probably clarify the situation for you here. But also would be a pain in other ways.)
Also you could probably use JuliaSyntax's flags to simplify some things.
elseif JuliaSyntax.is_prec_comparison(nkind) && JuliaSyntax.is_trivia(node); | ||
:julia_comparator | ||
elseif isplainoperator(node); :julia_operator | ||
elseif nkind == K"..." && JuliaSyntax.is_trivia(node); :julia_operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing might be cleaner if you split the big elseif
thing into two sections based on haschildren()
?
Because, for example, K"for"
means two different things, depending on whether you're looking at a token (!haschildren()
- ie, the literal for
trivia token in the source) vs an interior node of the tree representing a for loop, with the comparision and body as children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, that's a good point. I might leave this for a future refactor (I'm sure there will be a few updates from this initial rewrite) though.
a533511
to
d33ae74
Compare
Let's not let the perfect be the enemy of improvement 🙂, I've moved some of the non-JuliaSyntax work that had collected here to #4, and I'd be keen to continue working on improving the highlighting across issues/future PRs. |
Previously we were just using the Tokenizer from JuliaSyntax, but we might as well use the full parse-tree. It's higher-quality data, and with just a little more work we can have much more accurate highlighting.
Subject to change as I work on this, but here's a before and after screenshot for the current progress:
Before
After
Specific showcases