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

enh(javascript) Added node-repl for Node.js REPL sessions #2792

Merged
merged 3 commits into from Nov 11, 2020

Conversation

nagayev
Copy link
Contributor

@nagayev nagayev commented Oct 26, 2020

Issue: none

Changes

Add javascript-repl language with tests.

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md
  • Added myself to AUTHORS.txt, under Contributors

Copy link
Member

@joshgoebel joshgoebel left a comment

Awesome! Almost good to go.

src/languages/javascript-repl.js Outdated Show resolved Hide resolved
test/markup/javascript-repl/sample.txt Outdated Show resolved Hide resolved
src/languages/javascript-repl.js Outdated Show resolved Hide resolved
@joshgoebel joshgoebel changed the title Add javascript-repl add(javascript-repl) Grammar to handle the REPL output of Node.js, etc... Oct 27, 2020
@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Oct 27, 2020

I'm fine with adding a repl grammar in general since we did this recently with Python also and it's my opinion that the repl's are better broken out than being mixed with the main languages (though we still have some mixed).

Though I wonder if the REPL isn't really node-repl... we have Deno also, but that's Typescript? Also is there a SpiderMonkey JS run-time repl also? @egor-rogov @allejo Any thoughts here? Would be nice to avoid the need for two repls (one for JS and one for TS)... So just wondering about two things:

  • The exact name for this grammar.
  • JS/TS support/Deno support, etc...
  • Other JS runtime REPLS... (SpiderMonkey, etc.)

While we're here might as well try and think about the bigger picture.

@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Oct 27, 2020

@joshgoebel Is it okey?
sample.txt

> for(let i=0;i<5;i++){
... console.log(i);
... }
0
1
2
3
4
undefined
> 

@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Nov 2, 2020

@joshgoebel I will fix CI build tommorow.

@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Nov 3, 2020

@joshgoebel How can I generate correct sample.exprect.txt?

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Nov 3, 2020

How can I generate correct sample.exprect.txt?

Well technically your'e supposed to write it by hand :-) ...but if you're very careful there is a commented line in test/markup/index.js that will overwrite ALL the tests with what the library THINKS the output should be.

@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Nov 3, 2020

@joshgoebel tkanks!

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Nov 3, 2020

Not sure you got that merge/rebase correct...

@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Nov 3, 2020

@joshgoebel Why?
I just copypasted AUTHORS and CHANGES from master and inserted myself.

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Nov 3, 2020

I see a bunch of php/ruby stuff now that has nothing to do with this PR.

@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Nov 3, 2020

@joshgoebel Please check my final version.

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Nov 3, 2020

Looks nice. My only concern is still the naming (there is really not general JS repl, it's the node repl), so I'm curious if anyone will chime in with any thoughts there.

src/languages/javascript-repl.js Outdated Show resolved Hide resolved
@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Nov 3, 2020

@joshgoebel
Hmm.
I was thinking of this name, node-repl. So I can rename language and maybe no one thinks otherwise.

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Nov 3, 2020

I was thinking of this name, node-repl. So I can rename language and maybe no one thinks otherwise.

But I reallly do not want node-repl, spidermonkey-repl, etc... If there are other repl's I'd probably want to kill them with this same stone (if possible, which prolly is)... which makes more of an argument for js-repl. :-) I'd just like to see if any of the other core team members have any thoughts.

@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Nov 8, 2020

@joshgoebel No response from another members of core team...
So isn't it time to merge?
My PR regularly has conflicts because it has been open for quite a long time.
So I suggest merging this PR after resolving conflict with AUTHORS and, if necessary, changing the js-repl to a different name.

@joshgoebel joshgoebel added this to the 10.4 milestone Nov 8, 2020
@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Nov 8, 2020

My PR regularly has conflicts because it has been open for quite a long time.

@nagayev Not a worry anymore. I can handle any final merge conflicts as it's just the changelog and authors. Zero more effort required on your part.

I tagged this 10.4 so it will go into the next release. There isn't a hurry to merge until then. I've still been noodling on the name myself as well.

@nagayev
Copy link
Contributor Author

@nagayev nagayev commented Nov 8, 2020

@joshgoebel All right

@joshgoebel joshgoebel changed the title add(javascript-repl) Grammar to handle the REPL output of Node.js, etc... enh(javascript) Added node-repl for Node.js REPL sessions Nov 11, 2020
@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Nov 11, 2020

Since you were working on master you may want to hard reset your master to upstream/master now that this is merged to make future contributions easier. And I'd recommend branching first in the future to avoid the need for this step. :)

I also removed this from common as that is a different discussion.

Thanks so much! This will go out in 10.4 (later this month prolly).

@joshgoebel joshgoebel merged commit 435f6c6 into highlightjs:master Nov 11, 2020
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants