-
-
Notifications
You must be signed in to change notification settings - Fork 485
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
feat: add APIs, commands and tests for syntax-highlighting in the REPL #2291
Conversation
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>
@Snehil-Shah Another possibility for a couple of the commands which should have corresponding settings:
and then
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 |
@kgryte I don't think we would need a |
Re: Re: If you are thinking the prototype method, maybe we should do |
@kgryte I think having |
Currently, the main precedent for get/set APIs is Looking through the codecase, we don't have many instances of public classes. The closest one which has getters/setters is While overloaded getter/setter APIs is a fairly common design pattern, I'm also okay with separate commands: |
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Updated Signatures- REPL commands
- Prototype methods
|
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>
Did some changes and fixed some bugs that I encountered while writing the tests as explained in the commit descriptions. |
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
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>
bab4bab
to
7c99ef7
Compare
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah Yes, I'd suggest renaming the default theme to something else (e.g., |
@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>
There was a problem hiding this 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>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
There was a problem hiding this 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!
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>
Description
This pull request:
- Adds the following REPL commands:
theme([name])
- Set theme or list current and available themesaddTheme(name,colors)
- add user-defined themedeleteTheme(name)
- delete a themedefaultTheme([name])
- Set/get the default themegetTheme([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:
Related Issues
This pull request:
Questions
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.Similarly with
defaultTheme
. Is that okay?Other
No.
Checklist
@stdlib-js/reviewers