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: add APIs, commands and tests for syntax-highlighting in the REPL #2291

Merged

Conversation

Snehil-Shah
Copy link
Member

@Snehil-Shah Snehil-Shah commented Jun 1, 2024

Description

What is the purpose of this pull request?

This pull request:

- Adds the following REPL commands:

  • theme([name]) - Set theme or list current and available themes

  • addTheme(name,colors) - add user-defined theme

  • deleteTheme(name) - delete a theme

  • defaultTheme([name]) - Set/get the default theme

  • getTheme([name]) - Get the JSON of the current or a specified theme

- Adds API methods to the REPL instance (like repl.setTheme() etc.) - to programmatically control the syntax-highlighter.

- Adds setting to disable/enable the syntax highlighter

- Add options to load themes and set default theme upon REPL startup

- Fixes failing tests

TODO:

  • Tests
  • more built-in themes
  • document the new API methods

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

  • I have added a long list of API methods to the REPL instance, is that okay or a bit of an overkill?
  • I have combined a couple of operations when writing REPL commands as it was getting crowded.
    Like the theme command both prints the current and available themes when no argument is provided and sets the theme when an argument is provided.
    image
    Similarly with defaultTheme. Is that okay?

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@kgryte
Copy link
Member

kgryte commented Jun 2, 2024

@Snehil-Shah Another possibility for a couple of the commands which should have corresponding settings:

# Set the default theme:
> settings( 'defaultTheme', 'foo' )

# Set the current theme:
> settings( 'theme', 'foo' )

and then

# Add a theme:
> addTheme( 'bar', {...} );

# Remove a theme: (similar to existing `deleteWorkspace` command)
> deleteTheme( 'bar' );

# List themes: (similar to existing `workspaces()` command)
> themes()
'monaco', 'foo'

# Return the current theme: (similar to existing `currentWorkspace` variable)
> currentTheme

# Rename a theme: (similar to existing `renameWorkspace()` command)
> renameTheme( 'foo', 'beep' );

# Check whether a theme exists: (similar to existing `isWorkspace()` command)
> isTheme( 'beep' )
true

In short, I'd favor consistency with conventions that we already have in terms of naming and the set of workspaces commands is the closest match.

For REPL prototype methods, I think we may be able to combine separate get/set methods into single methods (e.g., defaultTheme( [name] )).

@Snehil-Shah
Copy link
Member Author

Snehil-Shah commented Jun 2, 2024

@kgryte I don't think we would need a currentTheme if we are going to have a setting for the current theme, as settings already allows us to get the current value of a setting. And what about getThemeConfig?

@kgryte
Copy link
Member

kgryte commented Jun 2, 2024

Re: currentTheme. Right. Makes sense: settings( 'theme' ) would return the current theme.

Re: getThemeConfig. If you are thinking a command, maybe since addTheme( name, config ), it would be more consistent to have the command name be getTheme( name ) and return a config object. Meaning, getThemeConfig => getTheme. I believe this is what you originally proposed. Or maybe we should just consolidate here and do theme( name[, config] ), where theme( name ) returns a config object and theme( name, config ) adds a theme. This would be slightly different than what you originally proposed.

If you are thinking the prototype method, maybe we should do theme( name[, config] ), which is a get/set method. where the set behavior adds a theme. Then, to programmatically enable the newly added theme, REPL#settings( 'theme', 'foo' ).

@kgryte kgryte added the REPL Issue or pull request specific to the project REPL. label Jun 2, 2024
@Snehil-Shah
Copy link
Member Author

@kgryte I think having theme command for both getting the config and adding a new theme can be confusing. theme command can be easily confused to something like changing the current theme etc. I feel it's better to be explicit and have two commands addTheme and getTheme both as a command and as REPL prototype methods. What do you think?

@kgryte
Copy link
Member

kgryte commented Jun 2, 2024

Currently, the main precedent for get/set APIs is settings( [name[, value]] ). It doesn't look like we have other similar commands atm. And we don't have other similar prototype methods.

Looking through the codecase, we don't have many instances of public classes. The closest one which has getters/setters is @stdlib/plot/ctor, but the needs are slightly different there.

While overloaded getter/setter APIs is a fairly common design pattern, I'm also okay with separate commands: getTheme and addTheme.

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah
Copy link
Member Author

Updated Signatures

- REPL commands

  • themes() - List available themes
  • addTheme(name,value) - Add user-defined theme
  • deleteTheme(name) - Delete a theme
  • renameTheme(old,new) - Rename a theme
  • getTheme([name]) - Get JSON config of the current or a specified theme
  • settings('theme') - Set/get current theme
  • settings('defaultTheme') - Set/get default theme

- Prototype methods

  • REPL#themes() - Get all themes
  • REPL#getTheme(name) - Get a specified theme's JSON config
  • REPL#addTheme(name,value) - Add a theme
  • REPL#renameTheme(old,new) - Rename a theme
  • REPL#deleteTheme(name) - Delete a theme
  • REPL#settings('theme') - Set/get current theme
  • REPL#settings('defaultTheme') - Set/get default theme

Following IDE conventions like VSCode

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Found the bug when testing against `try{} catch` as `resolveLocalScopes` was returning null nodes.
This is not specific to the syntax highlighter but specific to the function. You can also reproduce the bug by turning off the syntax highlighter and clicking TAB after writing the above expression

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
When writing an expression like (4) => , it was throwing an error as the declaration pattern was of type `Literal` and the default case was raising an error.

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah
Copy link
Member Author

Did some changes and fixed some bugs that I encountered while writing the tests as explained in the commit descriptions.

@Snehil-Shah Snehil-Shah marked this pull request as ready for review June 2, 2024 15:00
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah
Copy link
Member Author

Snehil-Shah commented Jun 2, 2024

I will add more built-in themes in another small PR once this is merged, or should I push it in this PR itself? Also, I was thinking if we should keep the name of the default in-built theme (currently 'default') to something less boring. How about something like 'stdlib'?

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah Snehil-Shah force-pushed the syntax-highlighting-integrations branch from bab4bab to 7c99ef7 Compare June 3, 2024 00:54
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@kgryte
Copy link
Member

kgryte commented Jun 3, 2024

@Snehil-Shah Yes, I'd suggest renaming the default theme to something else (e.g., stdlib-light, as the current theme is better on light backgrounds and is not great on black terminal backgrounds). And for any built-in stdlib themes which don't have well-known names (e.g., monaco, etc), we can use the "stdlib-" naming convention.

@kgryte
Copy link
Member

kgryte commented Jun 3, 2024

@Snehil-Shah I suggest adding additional themes in one or more follow-up PRs.

Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@Snehil-Shah Looks like just a couple of small things remain to get this PR over the finish line. Namely, some docs need updating, some small refactoring, and a question on printed confirmation messages.

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah Snehil-Shah requested a review from kgryte June 8, 2024 04:31
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

Okay. I think this is ready to merge. Any further updates can be made in follow-up PRs. Thanks, @Snehil-Shah!

@kgryte kgryte merged commit b4c12b7 into stdlib-js:develop Jun 8, 2024
7 checks passed
aman-095 pushed a commit to aman-095/stdlib that referenced this pull request Jun 13, 2024
PR-URL: stdlib-js#2291
Ref: stdlib-js#2254
Co-authored-by: Athan Reines <kgryte@gmail.com>
Reviewed-by: Athan Reines <kgryte@gmail.com> 
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Athan Reines <kgryte@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Issue or pull request specific to the project REPL.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants