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

feat: Highlight in prqlc #4755

Merged
merged 10 commits into from
Jul 21, 2024
Merged

feat: Highlight in prqlc #4755

merged 10 commits into from
Jul 21, 2024

Conversation

vanillajonathan
Copy link
Collaborator

This PR adds a command called highlight (under experimental) to prqlc which does syntax highlighting on the terminal.

Example:

prqlc experimental highlight example.prql

It works pretty good but single quotes becomes double quotes because that's what the lexer does.

I couldn't get the test to work because somehow it still includes the ANSI escape sequences even though the test sets the environment variable NO_COLOR and passes in the --color=never argument. It shows a diff between what is returned and what is expected that is identical.

-␛[32mfrom␛[39m foo
+␛[32mfrom␛[39m foo

Later maybe we can use this in prqlc compile too.

@max-sixty
Copy link
Member

Very cool! I will look tomorrow more.

We could instead instead enable this by default for prqlc fmt...

Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

I think this is great! Nice work!

Can you push a version with a test? If the color stuff breaks then I will take a look and try to fix; but we do need a test.

I added some notes — let's have the linewrap passing. The TODO you don't need to do now, but let's leave it in the code for the future.


Something we could do is run this automatically on the output of prqlc fmt, rather than have this behind the experimental option. We could even add a --no-fmt option there which only does highlighting.

@max-sixty
Copy link
Member

@vanillajonathan this is really nice, for example:

image

I refactored some related code (maybe I should have done it in a separate PR, sorry to use yours...). Take a look at the changes and then hit the button if you're happy

@vanillajonathan
Copy link
Collaborator Author

Do want it to still reside under experimental?
Feel free to refactor it into fmt if you feel it makes more sense to have it there.
The codecov fails.

The changes look fine. I think you should merge it!

@max-sixty
Copy link
Member

max-sixty commented Jul 21, 2024

Do want it to still reside under experimental?
Feel free to refactor it into fmt if you feel it makes more sense to have it there.

Let's merge now as experimental and then we can refactor in into fmt later?

The codecov fails.

Yes, another advantage of merging with fmt is we'll get full test coverage given that's heavily tested... Doesn't need to stop us merging though


I'll hit the button now. If you want to add a Changelog as a separate PR that would be nice, so people know about it

@max-sixty max-sixty enabled auto-merge (squash) July 21, 2024 19:17
@max-sixty max-sixty merged commit 577ae6f into main Jul 21, 2024
35 of 36 checks passed
@max-sixty max-sixty deleted the highlight branch July 21, 2024 19:19
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.

2 participants