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 NUMBER and BOOLEAN tokens #123

Merged
merged 4 commits into from
May 23, 2024
Merged

Conversation

bogdancondorachi
Copy link
Contributor

@bogdancondorachi bogdancondorachi commented May 16, 2024

Hey @brendt, I know you said in #96 that you don't plan on adding new tokens so this might be pointless.

But I'm still taking my shoot on implementing NUMBER and BOOLEAN tokens because:

  • they are an important part of syntax highlighting and deserve their own tokens
  • most themes handle them differently, making it impossible to highlight them correctly on multi-theme setup

If approved, I'm happy to do another PR on updating the themes/classes with these new tokens.

@coveralls
Copy link

coveralls commented May 16, 2024

Pull Request Test Coverage Report for Build 9206597206

Details

  • 18 of 18 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 95.429%

Totals Coverage Status
Change from base Build 9205225309: 0.02%
Covered Lines: 1503
Relevant Lines: 1575

💛 - Coveralls

@bogdancondorachi bogdancondorachi mentioned this pull request May 16, 2024
3 tasks
@SRWieZ SRWieZ mentioned this pull request May 16, 2024
3 tasks
@brendt
Copy link
Member

brendt commented May 22, 2024

I'm fine merging this in, but we cannot update existing languages without it being a breaking change: if a specific word changes to use a new class while people don't have this CSS class present in their stylesheet, their highlighting will break.

So I'm fine merging this in, and you can use it in newly added languages like #121, but we won't be able to update new languages until a later point.

We should also update LightTerminalTheme to include these changes, can you add those changes to this PR as well?

@brendt brendt merged commit c6cf259 into tempestphp:main May 23, 2024
4 checks passed
@brendt
Copy link
Member

brendt commented May 23, 2024

Thanks!

@brendt
Copy link
Member

brendt commented May 23, 2024

@bogdancondorachi
Copy link
Contributor Author

@brendt I've updated the LightTerminalTheme with the added tokens.

So I'm fine merging this in, and you can use it in newly added languages like #121, but we won't be able to update new languages until a later point.

Now this is perfectly fine for me, though I believe it's not really a breaking change if we are to update current languages to include these tokens as well as it all depends on the user CSS, basically if token CSS is missing it would simply not add the respective color, leaving it with a blank class if I'm correct. I see it more as a missing feature if they don't update their CSS.

Steps I'm seeing are as follows:

  • Update themes at first (some also needs color correction)
  • Update languages where necessary

You can ping me on Discord to discuss more on this if you want. :)

bogdancondorachi added a commit to bogdancondorachi/highlight that referenced this pull request May 23, 2024
bogdancondorachi added a commit to bogdancondorachi/highlight that referenced this pull request May 23, 2024
bogdancondorachi added a commit to bogdancondorachi/highlight that referenced this pull request May 23, 2024
@bogdancondorachi bogdancondorachi deleted the new-tokens branch May 23, 2024 11:16
@bogdancondorachi bogdancondorachi restored the new-tokens branch May 23, 2024 11:16
@bogdancondorachi bogdancondorachi deleted the new-tokens branch May 23, 2024 11:17
@brendt
Copy link
Member

brendt commented May 23, 2024

bogdancondorachi added a commit to bogdancondorachi/highlight that referenced this pull request May 28, 2024
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