Skip to content
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

Remove highlighting on various non-top-level builtins #53

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

haltman-at
Copy link
Collaborator

So I figure, as long as we're already considering a 2.0 release due to #37, why not remove highlighting on some things that shouldn't be highlighted?

Since before I started working on this, call and other address members have been highlighted as top-level builtins. They're not. The justification for this highlighting is that, in Solidity versions prior to 0.5.0, if you used one of these as a name for a function, it could cause problems for you, so they were highlighted as if they were top-level builtins to point this out.

Well, Solidity 0.4.x isn't used that much anymore (let alone any Solidity prior to that). Also, highlightjs isn't even really suited to being used in the sort of situations where that sort of highlighting would be relevant; as best I can tell, it really is for highlighting source files that are already complete, not ones that are in the process of being written. E.g., it doesn't provide a way to highlight illegal syntax; its illegal mechanism is for saying "nope that can't be this language", not for saying "you should highlight this as forbidden". That's why highlightjs-solidity doesn't for instance bother including Solidity's long list of reserved-but-unused keywords in its highlighting.

So, I think we can remove these by this point. They're already highlighted as builtins in their appropriate context by other mechanisms; there's no need for them to be highlighted in all contexts.

I'd remove the highlighting on self as well, but, well, that's a separate issue and I expect a more contentious one, so I figure let's just leave that alone for now...

(Also, I just noticed that there wasn't a space between log4 and send, meaning that rather than highlighting the [rarely used and now removed] log4 builtin and the [not a real builtin as per above] send builtin, we were instead highlighting the identifier log4send, if anyone ever happened to use such a thing! Uh, if we don't merge this, then we should instead merge a fix for that.)

@joshgoebel
Copy link
Member

Also, highlightjs isn't even really suited to being used in the sort of situations where that sort of highlighting would be relevant; as best I can tell, it really is for highlighting source files that are already complete, not ones that are in the process of being written. E.g., it doesn't provide a way to highlight illegal syntax;

I dunno if I would say "ill-suited" exactly. :-) I've considered adding an illegal scope (for 3rd party use)... but it's not something we're interested in getting into for first-part grammars... that said if a 3rd party grammar wanted to add such a scope (and CSS for it) with various rules it would work just like any other rules. We (as a library) just don't see the benefit in highlighting illegal code - it's more of an edge case concern.

Though since we're a pattern matcher illegal is a bit harder to find than say with a very very language aware parser than can just highlighting anything that doesn't match the full grammar... strict grammars would be easier to do than very flexible ones.

@haltman-at
Copy link
Collaborator Author

Sure. Mostly I just want to remove these because, well, they shouldn't be there; I'd remove self as well except I expect someone would object. :P The bit about illegal stuff is just an additional argument as to why.

Copy link
Collaborator

@eggplantzzz eggplantzzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me

@haltman-at
Copy link
Collaborator Author

OK, let's do this!

@haltman-at haltman-at merged commit 8afb39e into master Aug 25, 2021
@haltman-at haltman-at deleted the dehighlight branch August 25, 2021 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants