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

Update python highlighting to take advantage of themes. #2451

Merged
merged 13 commits into from
May 20, 2022

Conversation

Zeddicus414
Copy link
Contributor

@Zeddicus414 Zeddicus414 commented May 10, 2022

This is the work after this conversation #2427

Contributed by @Zeddicus414:

  • Separate @keyword and @keyword.control

Contributed by @paul-scott:

  • str, list, etc. handled as @function.builtin and @type.builtin
  • None and non-conforming type indentifiers as @type in type hints
  • class identifiers treated as @type
  • @constructor used for constructor definitions and calls rather than
    as a catch-all for type-like things
  • Parameters highlighted
  • self and cls as @variable.builtin
  • improved decorator highlighting as part of @function

Re-ordering of some statements to give more accurate priority.

Before After
image image

Create type keywords
Allow _CONSTANTS to start with _
Highlight constants before constructors
Move some keywords into @keyword.control

; Builtin functions

((call
function: (identifier) @function.builtin)
(#match?
@function.builtin
"^(abs|all|any|ascii|bin|bool|breakpoint|bytearray|bytes|callable|chr|classmethod|compile|complex|delattr|dict|dir|divmod|enumerate|eval|exec|filter|float|format|frozenset|getattr|globals|hasattr|hash|help|hex|id|input|int|isinstance|issubclass|iter|len|list|locals|map|max|memoryview|min|next|object|oct|open|ord|pow|print|property|range|repr|reversed|round|set|setattr|slice|sorted|staticmethod|str|sum|super|tuple|type|vars|zip|__import__)$"))
Copy link
Member

Choose a reason for hiding this comment

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

These are built in functions though (if used with parentheses) https://www.programiz.com/python-programming/methods/built-in/str

VSCode highlights these differently, but I'd argue our highlighting is more precise here since we can distinguish between a type and a built-in function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true, however, I would respectfully argue that the type changing properties of these functions and their uses for type checking tools like Mypy and Typeguard more closely relate these particular functions to types.

At least in the codebases I am using, these functions are increasingly called to clarify types for type checkers as much as to change types at runtime.

Copy link
Contributor

@paul-scott paul-scott May 11, 2022

Choose a reason for hiding this comment

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

Independently of this (only just found this PR now), I implement some changes to the python highlighting. This touches on type highlighting among other things (parameters, decorators, etc.). I managed to treat str and the like as @function.builtin when being used as a function, and treat them as @type when being used in type hints. See #2457

Copy link
Contributor

Choose a reason for hiding this comment

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

@Zeddicus414 if you think this is ready, go ahead and mark this PR as open again.

We've merged in the highlights work in #2457. archseer one of the changes keeps str, int, list, and so on as @function.builtin for the cases where they are being called, otherwise they are treated as @type.builtin. So hopefully that addressed your concern. Hopefully the sequence of commits is not too messy to follow, otherwise let us know if we need to clean things up.

@@ -6,7 +6,7 @@
"type" = { fg = "type" }
"type.builtin" = { fg = "type" }
"type.enum.variant" = { fg = "constant" }
"constructor" = { fg = "constant" }
"constructor" = { fg = "type" }
Copy link
Member

Choose a reason for hiding this comment

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

\cc @kirawi to confirm this

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think that the usage of @constructor has become somewhat conflated with @type as in many highlight queries it applies to all identifiers with ^[A-Z]. I'm fine with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

The recent commits we made to this PR fixed up the usage of @constructor so it is no longer conflated with @type in python at least. I suspect originally that was the main motivation for @Zeddicus414 wanting to change this. For style reasons, maybe the change here is still a good idea. I just thought I'd point this out.

@Zeddicus414 Zeddicus414 marked this pull request as draft May 12, 2022 13:31
Paul Scott and others added 8 commits May 13, 2022 16:21
* str, list, etc. handled as @function.builtin and @type.builtin
* None and non-conforming type indentifiers as @type in type hints
* class identifiers treated as @type
* @constructor used for constructor definitions and calls rather than
  as a catch-all for type-like things
* Parameters highlighted
* self and cls as @variable.builtin
* improved decorator highlighting as part of @function

Re-ordering of some statements to give more accurate priority.
Python highlight improvements: type, parameter etc
Highting parameters with defaults and keyword arguments
Python highlight decorator attribute
@Zeddicus414 Zeddicus414 marked this pull request as ready for review May 15, 2022 16:58
@the-mikedavis the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label May 18, 2022
@archseer archseer merged commit 09f9f70 into helix-editor:master May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants