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

Add a theme to vscode-nim? #142

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add a theme to vscode-nim? #142

wants to merge 4 commits into from

Conversation

AMSwift73
Copy link

This follows up on my earlier issue report and on the Nim forum thread https://forum.nim-lang.org/t/12697.

Proposal: Add nim-ready themes to the vscode-nim extension. The theme offered is still in beta and needs review, but it does:

  1. Recognize Nim code tokens
  2. Helpfully colorize indentation levels, so important in Nim, and
  3. Ensure that only Nim brackets are colorized by the "colorize bracket level" option.

I apologize for the messy commit history; I'm new to git, github, and the github desktop app.
readme.md should be unchanged, package.json should only have a block added pointing to a theme (near end of file), and the only significant addition is one theme in a new "themes" folder.

AMSwift73 and others added 4 commits March 12, 2025 10:42
Theme is a work in progress. It is provided as an example.
Restore readme.md
@jmgomez
Copy link
Collaborator

jmgomez commented Mar 12, 2025

Why this needs to be part of the extension though?

@AMSwift73
Copy link
Author

There is no strict need for one or more themes to be part of the standard Nim extension.

However, one part of this extension that really IS needed - and already present - is the correct recognition of Nim tokens, including indentation and brackets. Adding themes that more fully translate this vital job into visuals for developers helps complete this core task.

Therefore, this PR intended to be a) small, b) relatively unimportant in itself, and c) usefully support a much more important job.

@Araq
Copy link
Member

Araq commented Mar 13, 2025

Does this do the tokenization properly? A good test case is system.nim after line 145:

  proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {.
    magic: "NewFinalize", noSideEffect, deprecated: "pass a finalizer of the 'proc (x: T) {.nimcall.}' type".}

Messes up my current tooling because of the pragma syntax used inside a string literal inside a pragma.

@AMSwift73
Copy link
Author

Araq,
Quick Q: Do you expect the whole string "pass a finalizer...", including "{.nimcall.}", to be of a uniform color - as displayed here on github - or that the entire pragma {.magic: ... type.} be a uniform color, or am I misunderstanding?

If the former, then vscode with vscode-nim should already accommodate you? If the latter, that's (at least) a syntax file issue; changing only the theme will not help.

@Araq
Copy link
Member

Araq commented Mar 13, 2025

Quick Q: Do you expect the whole string "pass a finalizer...", including "{.nimcall.}", to be of a uniform color

Yes, this one. There is not much complexity going on here, just tokenize as always inside {. .} -- like they would be ( ).

@jmgomez
Copy link
Collaborator

jmgomez commented Mar 13, 2025

@AMSwift73 can you provide some screenshots showing how this actually helps? Bonus points if you also make the PR so it doesnt modify existing lines but only your actual changes

@jmgomez
Copy link
Collaborator

jmgomez commented Mar 13, 2025

Does this do the tokenization properly? A good test case is system.nim after line 145:

  proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {.
    magic: "NewFinalize", noSideEffect, deprecated: "pass a finalizer of the 'proc (x: T) {.nimcall.}' type".}

Messes up my current tooling because of the pragma syntax used inside a string literal inside a pragma.

@Araq doesnt yours look like this?
image

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