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

(shell) Fix formatting when '/' is in a shell path #2218

Merged
merged 2 commits into from
Oct 24, 2019

Conversation

z4ce
Copy link
Contributor

@z4ce z4ce commented Oct 22, 2019

Right now if you have a prompt like bash /home/user#
the syntax highlighter doesn't correctly detect the prompt.
This commit fixes that by adding / to the character set for
prompt detection

@joshgoebel
Copy link
Member

joshgoebel commented Oct 22, 2019

Please include some tests and update the changelog. :-)

How does this differential between something like:

> /bin/true

Where the / is part of the command, not the prompt? I think the ^ solves that but please add a markup test to show this working as expected.

@joshgoebel
Copy link
Member

I'd love to get this in the next release if you could make the requested changes/additions.

@joshgoebel joshgoebel added this to the 9.15.12 milestone Oct 24, 2019
@joshgoebel joshgoebel changed the title Fix formatting when '/' is in a shell path (shell) Fix formatting when '/' is in a shell path Oct 24, 2019
@z4ce
Copy link
Contributor Author

z4ce commented Oct 24, 2019

I added a test, but I'm having trouble getting mocha to run with some of the relative require statements. Is there some magic NODE_PATH that needs to be set? Here is an example of what I am seeing:

╰─ npm test

> highlight.js@9.15.10 test /git/highlight.js
> mocha --globals document test

/git/highlight.js/node_modules/yargs/yargs.js:1163
      else throw err
           ^

Error: Cannot find module '../../build'

@joshgoebel
Copy link
Member

joshgoebel commented Oct 24, 2019

I'm not sure what you're doing... all you need to do is drop two files into:

/test/markup/shell

Look at the other languages for examples. You shouldn't need to add any actual JS code. Or if you mean you can't run the tests at all, then I'm not sure. npm test should work from the main checkout dir. Did you npm install everything?

OH you do need to build the "binary first":

node tools/build.js -t node

@z4ce z4ce force-pushed the master branch 2 times, most recently from 1f5882e to 1d798cc Compare October 24, 2019 03:30
@z4ce
Copy link
Contributor Author

z4ce commented Oct 24, 2019

Thanks! That was the issue. I had built for browser but not for node.

Copy link
Member

@joshgoebel joshgoebel left a comment

Choose a reason for hiding this comment

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

Looks good, just need a changelog entry and this should be shippable!

Right now if you have a prompt like `bash /home/user#`
the syntax highlighter doesn't correctly detect the prompt.
This commit fixes that by adding / to the character set for
prompt detection

Co-Authored-By: Shishir Amin <samin@pivotal.io>
@z4ce
Copy link
Contributor Author

z4ce commented Oct 24, 2019

Updated the changelog

@z4ce
Copy link
Contributor Author

z4ce commented Oct 24, 2019

It looks like the CI system errored and no tests actually failed

@joshgoebel
Copy link
Member

Yikes.

@joshgoebel
Copy link
Member

@z4ce Thanks so much for contributing!

@joshgoebel joshgoebel merged commit 5b413a6 into highlightjs:master Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants