+ support for multi-language 3rd-party packages #2336

ericprud · 2019-12-26T14:11:45Z

tested with:
node ./tools/build.js -t all plaintext
node ./tools/build.js -t all plaintext shexc solidity sparql ttl

not tested for with test/detect or test/markup tests ('cause I don't
know how).

Requires highlightjs/highlight.js#2336 to build. TODO: harmonize URL productions between the three modules. harmonize classNames between the three modules.

joshgoebel · 2019-12-26T15:26:35Z

not tested for with test/detect or test/markup tests ('cause I don't
know how).

If the build is working then just running the full test suite should ALSO test your submodules. Is that not what you're seeing?

joshgoebel · 2019-12-26T15:44:00Z

Oh wait this is a PR lol.... :-) Let me take a closer look.

joshgoebel

See my two comments and tell me I'm not crazy. :-) Otherwise this looks pretty good and about what I'd expect.

joshgoebel · 2019-12-26T15:50:26Z

test/detect/index.js

@@ -34,7 +34,7 @@ function testAutoDetection(language, {detectPath}) {

 describe('hljs.highlightAuto()', () => {
  before( async function() {
-    let thirdPartyLanguages = await getThirdPartyLanguages();
+    let thirdPartyPackages = await getThirdPartyPackages(); // !! not used


It's used on line 49. listLanguages in the build already knows the FULL language list. We only need to figure out where the tests are at.

oops, was being blind

Can you remove the not used comments?

done.
Oh cool, the code snippit gets an [Outdated] annotation above it!

joshgoebel · 2019-12-26T15:51:10Z

tools/lib/language.js

@@ -9,7 +9,7 @@ const REQUIRES_REGEX = /\/\*.*?Requires: (.*?)\n/s
 const CATEGORY_REGEX = /\/\*.*?Category: (.*?)\n/s
 const LANGUAGE_REGEX = /\/\*.*?Language: (.*?)\n/s
 const {rollupCode} = require("./bundling.js")
-const { getThirdPartyLanguages } = require("./external_language")
+const { getThirdPartyPackages } = require("./external_language") // !! not used


Used on line 101.

joshgoebel · 2019-12-26T16:00:05Z

Maybe look at your one failing test though:

  1) hljs.highlightAuto()
324       "before all" hook for "adding dynamic tests...":
325     Error: detectTestDir(1c) not found

ericprud · 2019-12-26T18:13:27Z

i've been unable to run the tests; test/api/ident.js requires ../../build. I've tried to ./tools/build.js -t cdn and all and not seen an index.js show up. If I hack ident.js to require ../../build/highlight.js, I get an undefined hljs in build/highlight.js. I gave up with that one.

joshgoebel · 2019-12-26T18:18:36Z

You need a node build:

% git co squash_build_pipeline 
% npm install 
% node  --stack-size=65500  ./tools/build.js -t node  
Starting build.
Finished build.
Writing styles.
Writing package.json.
Writing languages.
................................................................................................................................................................................................
Writing highlight.js

% ./node_modules/.bin/mocha  -b false  --globals document test/

  hljs
    .IDENT_RE
...

ericprud · 2019-12-26T21:38:47Z

Weak. b0rk something there, didn't I?
notes for me to attack it in the morning:

git clone {git@github.com:highlightjs/,}highlight.js && cd $_
(mkdir extra && cd extra && git clone {git@github.com:highlightjs/,}highlightjs-shexc)
(git checkout squash_build_pipeline && npm install && node ./tools/build.js -t node)
./node_modules/.bin/mocha  -b false  --globals document test/

ends with yaml and shexc an passes 482.

git fetch origin +refs/pull/2336/merge && git merge FETCH_HEAD && node ./tools/build.js -t node
./node_modules/.bin/mocha  -b false  --globals document test/

passes 293, fails:

1) hljs.highlightAuto()
2) highlight() markup

This isn't the same error you got. Do you think I've screwed something up here?

tested with: node ./tools/build.js -t all plaintext node ./tools/build.js -t all plaintext shexc solidity sparql ttl not tested for with test/detect or test/markup tests ('cause I don't know how).

joshgoebel · 2019-12-27T14:31:13Z

Just FYI when you force push it's harder to review what you've changed...

highlightjs#2336 (comment) actually two errors: 1 autodetection codepath can return null for non-extra packages 2 'highlight() markup'::before wasn't finished TODO: hljs.highlightAuto()

ericprud · 2019-12-27T19:00:36Z

Re

Error: detectTestDir(1c) not found

, I'd not noticed this in the top of test/detect/index.js which makes it OK to have no detectPath:

function testAutoDetection(language, {detectPath}) {
  const languagePath = detectPath || utility.buildPath('detect', language);

For all of the non-extra languages, detectTestDir fell through to undefined (now null to make it a bit more explicit).

Just FYI when you force push it's harder to review what you've changed...

Oops, just pushed one I'd committed this AM. I've stopped generating them, though. (I can squash just before merge if you want.)

ericprud · 2019-12-27T21:36:39Z

should pass all tests with the highlightjs-shexc/master (482 tests) and highlightjs-shexc/multi-lang (484 tests).
last bug was sparql.js conflicting with sas 'cause it was too greedy with the keywords.

joshgoebel · 2019-12-28T00:39:18Z

now null to make it a bit more explicit).

Ok, that's fine. Though I don't much mind undefined being stand-in, could be my Ruby roots where we just have a single nil value.

I can squash just before merge if you want.)

No need, GitHub does that automatically if I want.

last bug was sparql.js conflicting with sas 'cause it was too greedy with the keywords.

LOL. I can't wait until people start making PRs that change our code because their tests are failing in their 3rd party grammar, but perhaps that's just a bad dream. :)

test/detect/index.js

Requires highlightjs/highlight.js#2336 to build. TODO: harmonize URL productions between the three modules. harmonize classNames between the three modules.

* + support for multi-language 3rd-party packages

joshgoebel reviewed Dec 26, 2019

View reviewed changes

joshgoebel added enhancement An enhancement or new feature package/build Issues relating to npm or packaging labels Dec 26, 2019

ericprud force-pushed the squash_build_pipeline branch 2 times, most recently from e7fb0a3 to f7821bd Compare December 27, 2019 09:06

+ support for multi-language 3rd-party packages

d41df87

tested with: node ./tools/build.js -t all plaintext node ./tools/build.js -t all plaintext shexc solidity sparql ttl not tested for with test/detect or test/markup tests ('cause I don't know how).

ericprud force-pushed the squash_build_pipeline branch from f7821bd to d41df87 Compare December 27, 2019 18:46

~ fix markup and detect errors

5a1df55

highlightjs#2336 (comment) actually two errors: 1 autodetection codepath can return null for non-extra packages 2 'highlight() markup'::before wasn't finished TODO: hljs.highlightAuto()

~ for the zillionth time, return doesn't work inside a forEach

115187c

Be explicit

6704865

joshgoebel approved these changes Jan 1, 2020

View reviewed changes

test/detect/index.js Outdated Show resolved Hide resolved

test/detect/index.js Show resolved Hide resolved

joshgoebel merged commit a55b39f into highlightjs:squash_build_pipeline Jan 1, 2020

ericprud deleted the squash_build_pipeline branch January 1, 2020 17:54

joshgoebel pushed a commit to joshgoebel/highlight.js that referenced this pull request Jan 19, 2020

+ support for multi-language 3rd-party packages (highlightjs#2336)

81393c8

* + support for multi-language 3rd-party packages

joshgoebel pushed a commit to joshgoebel/highlight.js that referenced this pull request Jan 19, 2020

+ support for multi-language 3rd-party packages (highlightjs#2336)

c1360bd

* + support for multi-language 3rd-party packages

joshgoebel pushed a commit to joshgoebel/highlight.js that referenced this pull request Jan 20, 2020

+ support for multi-language 3rd-party packages (highlightjs#2336)

e58a753

* + support for multi-language 3rd-party packages

joshgoebel pushed a commit to joshgoebel/highlight.js that referenced this pull request Feb 10, 2020

+ support for multi-language 3rd-party packages (highlightjs#2336)

69b91a9

* + support for multi-language 3rd-party packages

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

+ support for multi-language 3rd-party packages #2336

+ support for multi-language 3rd-party packages #2336

ericprud commented Dec 26, 2019

joshgoebel commented Dec 26, 2019

joshgoebel commented Dec 26, 2019

joshgoebel left a comment

joshgoebel Dec 26, 2019 •

edited

ericprud Dec 26, 2019

joshgoebel Dec 26, 2019

ericprud Dec 27, 2019

joshgoebel Dec 26, 2019

ericprud Dec 26, 2019

ericprud Dec 27, 2019

joshgoebel commented Dec 26, 2019

ericprud commented Dec 26, 2019

joshgoebel commented Dec 26, 2019 •

edited

ericprud commented Dec 26, 2019

joshgoebel commented Dec 27, 2019

ericprud commented Dec 27, 2019

ericprud commented Dec 27, 2019

joshgoebel commented Dec 28, 2019

+ support for multi-language 3rd-party packages #2336

+ support for multi-language 3rd-party packages #2336

Conversation

ericprud commented Dec 26, 2019

joshgoebel commented Dec 26, 2019

joshgoebel commented Dec 26, 2019

joshgoebel left a comment

Choose a reason for hiding this comment

joshgoebel Dec 26, 2019 • edited

Choose a reason for hiding this comment

ericprud Dec 26, 2019

Choose a reason for hiding this comment

joshgoebel Dec 26, 2019

Choose a reason for hiding this comment

ericprud Dec 27, 2019

Choose a reason for hiding this comment

joshgoebel Dec 26, 2019

Choose a reason for hiding this comment

ericprud Dec 26, 2019

Choose a reason for hiding this comment

ericprud Dec 27, 2019

Choose a reason for hiding this comment

joshgoebel commented Dec 26, 2019

ericprud commented Dec 26, 2019

joshgoebel commented Dec 26, 2019 • edited

ericprud commented Dec 26, 2019

joshgoebel commented Dec 27, 2019

ericprud commented Dec 27, 2019

ericprud commented Dec 27, 2019

joshgoebel commented Dec 28, 2019

joshgoebel Dec 26, 2019 •

edited

joshgoebel commented Dec 26, 2019 •

edited