add emacs #1297

JuanCaicedo · 2018-02-16T05:37:18Z

This adds the plugin developed by akirak/prism-emacs-lisp for the emacs lisp language. The extension is emacs.

I would like to support also lisp and elisp since both of those names are used, but I'm not sure what would be the best way to do that.

Let me know if there's anything missing to be able to add this 😀

JuanCaicedo · 2018-02-16T05:37:44Z

I'll try to post an example soon of this working

JuanCaicedo · 2018-02-16T06:43:28Z

You can see some of the highlighting at https://prism-emacs-ogfsnjvwlg.now.sh/post

Golmote

Hi! Thanks for contributing.

Apart from my inline comments, there are other things that need to be adressed before inclusion in Prism:

You need to add the language in the components.js file.
You should add an example file, following the format of the existing ones.
You should ideally add tests. You can find more information on the Test suite and look at the many existing examples.

Golmote · 2018-02-16T07:50:24Z

components/prism-emacs.js

@@ -0,0 +1,179 @@
+Prism.languages.emacs = (function () {


emacs is an editor before being a language. I think it would be less confusing to name the component emacs-lisp.

Since this is a dialect of Lisp, it would have been interesting to have generic support for Lisp first, and then maybe add specific dialects support if the generic component can't handle them.

Golmote · 2018-02-16T07:52:39Z

components/prism-emacs.js

+
+	// Symbol name. See https://www.gnu.org/software/emacs/manual/html_node/elisp/Symbol-Type.html
+	// & and : are excluded as they are usually used for special purposes
+	const symbol = '[-+*/_~!@$%^=<>{}\\w]+';


Prism is written is ES5. ES6 is not compatible with IE11 so this component would need to be transpiled or rewrittent in ES5. Here, this just means the const should be replaced with var.

The _ should not be needed here as it's already included in \w.

Golmote · 2018-02-16T07:53:56Z

components/prism-emacs.js

+	// Functions to construct regular expressions
+	// simple form
+	// e.g. (interactive ... or (interactive)
+	const simple_form = name => new RegExp(`(\\()${name}(?=[\\s\\)])`);


Same thing for fat arrows and template strings. Please use ES5.

There is no need to espace the closing parenthesis in the character class at the end.

Golmote · 2018-02-16T07:55:38Z

components/prism-emacs.js

+	// e.g. (interactive ... or (interactive)
+	const simple_form = name => new RegExp(`(\\()${name}(?=[\\s\\)])`);
+	// booleans and numbers
+	const primitive = pattern => new RegExp(`([\\s\\(\\[])${pattern}(?=[\\s\\)])`);


No need to espace the opening and closing parentheses in the character classes here.

Golmote · 2018-02-16T07:56:55Z

components/prism-emacs.js

+		},
+		'comment': /;.*/,
+		'string': {
+			pattern: /"(?:[^"\\]*|\\.)*"/,


What about line feeds? Are multiline strings supported in Emacs Lisp? If not, the character class should probably also exclude \r and \n.

I think multiline feeds are supported, I say that because this is a valid function definition

(defun multiply-by-seven (number) "Multiply NUMBER by seven, second line" (* 7 number))

Golmote · 2018-02-16T08:00:30Z

components/prism-emacs.js

+				pattern: primitive("[-+]?\\d+(?:\\.\\d*)?"),
+				lookbehind: true
+			}
+		],


Since the array contains only one pattern, it could be be removed.

Golmote · 2018-02-16T08:03:08Z

components/prism-emacs.js

+			pattern: new RegExp(par + 'def(?:var|const|custom|group)\\s+' + symbol),
+			lookbehind: true,
+			inside: {
+				'keyword': /def[a-z]+/,


I wonder what the highlighting would be here with a code piece like (defvar defabc? Maybe a ^ anchor should be added at the beginning of this regexp.

Golmote · 2018-02-16T08:04:23Z

components/prism-emacs.js

+			pattern: new RegExp(par + `(?:cl-)?(?:defun\\*?|defmacro)\\s+${symbol}\\s+\\([\\s\\S]*\\)`),
+			lookbehind: true,
+			inside: {
+				'keyword': /(?:cl-)?def\S+/,


A ^ anchor might be needed here too.

Golmote · 2018-02-16T08:05:25Z

components/prism-emacs.js

+			pattern: new RegExp(par + `lambda\\s+\\((?:&?${symbol}\\s*)*\\)`),
+			lookbehind: true,
+			inside: {
+				'keyword': /lambda/,


A ^ anchor might be needed here too.

JuanCaicedo · 2018-02-17T09:50:47Z

@Golmote Thanks for the feedback! I'm having some trouble understanding the tests. I'll upload here with some comments on what I'm having trouble with.

I think the renamings I made will cause the previous comments to appear outdated. I'm adding this here as a reminder that they still need to be addressed

JuanCaicedo · 2018-02-17T09:53:58Z

components.js

@@ -197,6 +197,10 @@ var components = {
 			"title": "Eiffel",
 			"owner": "Conaclos"
 		},
+		"elisp": {
+			"title": "Elisp",
+			"owner": "JuanCaicedo"


I'm not sure if I should be the owner, since I didn't develop this. Totally willing to defer to @akirak if they would like to take it over 😀

Thanks for creating this pull request.

I don't mind your being owner of the module, but this plugin is experimental, and there are some corner cases where it doesn't render well, like in this example. It's doing complex things, and I don't know how to fix this bug.

If you still don't mind using the plugin, you or whoever can be its owner. As you've put an effort in adapting the plugin, I think you deserve the credit. I will mention this merge in the README of my repo, but I won't be responsible for maintaining the plugin.

@akirak has a point, here. In Prism, we do expect the component's "owner" to do some kind of maintenance if issues regarding this component arise.
So you should probably keep it with your ownership, @JuanCaicedo.

JuanCaicedo · 2018-02-17T09:55:53Z

tests/languages/elisp/string_feature.test

+
+[
+	["string", ["\"\""]],
+	["string", "\"foo\\\r\nbar\""]


This leads to a failure which I don't understand

Token Stream: [["string",["\"\""]],"\r\n\"foo\\\r\nbar\""] ----------------------------------------- Expected Token Stream: [["string",["\"\""]],["string","\"foo\\\r\nbar\""]] -----------------------------------------

My impression is that the second case is not recognized as a string. I'm not sure if it's a problem I'm how I'm writing the test cases or in the implementation

I'm inclined to assume the implementation is right and my testing is wrong. If so, maybe the problem is that listing strings like this isn't valid lisp. Maybe the test tokens could me changed to

( "" "foo bar" )

Currently, your string pattern in the component allows only unescaped line feeds. #1297 (comment)

akirak · 2018-02-17T15:41:35Z

Also note, you may have to add some classes to your theme (CSS) to fully take advantage of it. I have tried it with Dracula theme included in the repo, which is a dedicated effort for the language, but I haven't tested it with any other themes for Prism.

akirak · 2018-02-18T03:31:05Z

@Golmote I'm the original author of the plugin, and I'm sorry for this tedious work. That code was not meant to be merged into the code base of PrismJS. It should be transpiled using babel, but apparently its resulting code will not be suitable for maintenance by human. Therefore I didn't send a merge request to you.

I am currently working on another project, and I'm not sure if I will adapt this stuff to the proper plugin format. Thank you for your comments.

akirak · 2018-02-18T03:47:50Z

The name emacs relates to a limitation of a blog engine I am using. My blog source contains code blocks in emacs-lisp language, but my blog engine somehow splits the name by hyphen, and the resulting class name becomes emacs. The plugin was named emacs so that it can highlight Emacs Lisp code in my blog, and I didn't consider supporting other Lisp dialects. Perhaps it should have been first named emacs-lisp, but I would still have to add an alias emacs for use in my blog.

Golmote · 2018-02-18T10:33:13Z

@akirak No worries. I noticed that some patterns are not handled by the official themes. It's not the first component doing so though, and it's ok if some parts are tagged but not styled. I'll just check the appearance once the PR is ready to be merged and maybe we'll make some adjustments. We might also want to add a comment with a link to your original repo, to credit your original work and in case some users are interested in using the Dracula theme you made.

The transpilation to ES5 should not be that tedious in this case: you're really just using tiny bits of syntactic sugar. Transforming const to var, fat arrows to regular functions, and template strings to good old string concatenation should be pretty straightforward.

I understand the naming limitation you had. Maybe we can still add emacs as an alias, since it seems to be the main scripting language used by this editor.

Thanks for showing up on the PR anyway! I hope @JuanCaicedo can take it to a mergeable state. :)

JuanCaicedo · 2018-04-05T15:33:05Z

@Golmote sorry for the delay, I'm working through these changes now 😀

JuanCaicedo · 2018-04-05T16:19:38Z

components/index.min.js

@@ -0,0 +1 @@
+function loadLanguages(e){e||(e=Object.keys(components.languages).filter(function(e){return"meta"!==e})),Array.isArray(e)||(e=[e]),e.forEach(function(e){components.languages[e]&&components.languages[e].require&&loadLanguages(components.languages[e].require),require("./prism-"+e)})}var components=require("../components.js");module.exports=loadLanguages;


This file was generated when I ran gulp. Not sure if it should be included. If not, I can revert the commit that added it

Oh! I never realized a minified version was generated for it.
I don't think it's needed, though. I'll have to update the gulpfile to exclude it. In the meantime, you can remove it from this PR, indeed.

Sorry about this, and thanks for pointing it out!

JuanCaicedo · 2018-04-05T16:20:17Z

components/prism-lisp.js

+	Prism.languages.lisp = language;
+	Prism.languages.elisp = language;
+	Prism.languages.emacs = language;
+	Prism.languages['emacs-lisp'] = language;


Not sure if this alias is allowed. I added it to allow more flexibility, but I can remove it you'd rather not have it

It's ok, we can keep it.

JuanCaicedo · 2018-04-05T16:24:18Z

@Golmote I fixed the merge conflicts and addressed the majority of the code comments (sorry for the accidental style changes 😬)

I'm working on trying to combine the regexes you mentioned. I'm finding it difficult though, so if it's okay to merge this without doing that, let me know 😀

Golmote

@JuanCaicedo Don't worry it's ok if you don't manage to merge the regexps. Maybe I'll try later, once the PR is merged.

Thanks for updating the PR, we're almost there! Please take a look at my comments.

Golmote · 2018-04-05T16:50:29Z

components/index.min.js

@@ -0,0 +1 @@
+function loadLanguages(e){e||(e=Object.keys(components.languages).filter(function(e){return"meta"!==e})),Array.isArray(e)||(e=[e]),e.forEach(function(e){components.languages[e]&&components.languages[e].require&&loadLanguages(components.languages[e].require),require("./prism-"+e)})}var components=require("../components.js");module.exports=loadLanguages;


Oh! I never realized a minified version was generated for it.
I don't think it's needed, though. I'll have to update the gulpfile to exclude it. In the meantime, you can remove it from this PR, indeed.

Sorry about this, and thanks for pointing it out!

Golmote · 2018-04-05T16:51:09Z

components/prism-lisp.js

+	Prism.languages.lisp = language;
+	Prism.languages.elisp = language;
+	Prism.languages.emacs = language;
+	Prism.languages['emacs-lisp'] = language;


It's ok, we can keep it.

Golmote · 2018-04-05T16:53:48Z

examples/prism-lisp.html

+	<li>"language-emacs"</li>
+	<li>"language-emacs-lisp"</li>
+</ul>
+


Due to a recent update, you can remove everything above this comment, in this file. The example header will be generated automatically.

Golmote · 2018-04-05T16:58:06Z

components.json

@@ -373,6 +373,11 @@
 			"title": "Liquid",
 			"owner": "cinhtau"
 		},
+		"lisp": {
+			"title": "lisp",


Can you capitalize the name here? "Lisp"

Note that you'll need to run gulp after this change, to update the Show Language plugin files.

Golmote · 2018-04-05T17:00:39Z

tests/languages/lisp/string_feature.test

+bar"
+"\"foo\""
+"foo\tbar"
+"foo\tbar"


Looks like there's a duplicate here. I don't think there's a need to test it twice.

Golmote · 2018-04-05T17:02:55Z

themes/prism.css

@@ -104,7 +104,7 @@ pre[class*="language-"] {
 .token.url,
 .language-css .token.string,
 .style .token.string {
-	color: #9a6e3a;
+	color: #a67f59;


This change should not be here. This file was changed 10 days ago in 8aea939. #9a6e3a is the correct value.

JuanCaicedo · 2018-04-05T17:20:01Z

@Golmote I've updated based on your last comments 😀

Golmote · 2018-04-05T17:24:51Z

@JuanCaicedo Yay! 🎉

add emacs

309d971

Golmote requested changes Feb 16, 2018

View reviewed changes

JuanCaicedo added 4 commits February 17, 2018 16:52

rename to elisp

2aee520

add elisp to components

5d3693b

add comment test

1c7021c

add heading test

4c96cab

WIP string test

68cd7b1

JuanCaicedo commented Feb 17, 2018

View reviewed changes

JuanCaicedo added 3 commits February 17, 2018 18:15

update string tests

1c45ad7

test symbols in strings

3fdd5a3

test arguments

e785a09

JuanCaicedo added 11 commits February 20, 2018 13:01

test quoted symbol

52a01d2

lisp-property test

83ba77a

splice test

399b39e

add keyword test

0ce2119

test for declare

f9dd612

test interactive

2a18699

boolean test

e60ef94

test numbers

f1551e8

test defvar

e0ad45e

fix greedy defun regex

46a21f7

test defun

994708e

JuanCaicedo added 2 commits April 5, 2018 10:27

rename example lisp file

a0099d4

move lisp to first position

95bc23d

JuanCaicedo added 10 commits April 5, 2018 10:47

explain null initialized properties in lisp file

fafc95b

remove trailing commas

a2d7b6e

put lisp first in example file

76c2cab

add ifee for lisp file

6b0f2c6

update lisp min file

5bd1817

update show language assets

c394ad7

add components index min file

0434ded

add emacs-lisp alias

ce3d0fa

change title of lisp example file

02ec9af

undo theme change

1201857

JuanCaicedo commented Apr 5, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/gh-pages' into add-emacs

feb38b6

combine regex for parens

857fb1f

Golmote requested changes Apr 5, 2018

View reviewed changes

JuanCaicedo added 6 commits April 5, 2018 12:15

remove min index

1aebecd

remove unneeded example html

58419f0

remove doubled string test

0b40f6f

capitalize lisp in components

7999c2b

change string theme to match master

9f29d39

build assets after adding lisp

ab77c09

Golmote approved these changes Apr 5, 2018

View reviewed changes

Golmote merged commit 46468f8 into PrismJS:gh-pages Apr 5, 2018

JuanCaicedo deleted the add-emacs branch April 6, 2018 19:31

		@@ -0,0 +1 @@
		function loadLanguages(e){e\|\|(e=Object.keys(components.languages).filter(function(e){return"meta"!==e})),Array.isArray(e)\|\|(e=[e]),e.forEach(function(e){components.languages[e]&&components.languages[e].require&&loadLanguages(components.languages[e].require),require("./prism-"+e)})}var components=require("../components.js");module.exports=loadLanguages;

add emacs #1297

add emacs #1297

Conversation

JuanCaicedo commented Feb 16, 2018

JuanCaicedo commented Feb 16, 2018

JuanCaicedo commented Feb 16, 2018 • edited Loading

Golmote left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuanCaicedo commented Feb 17, 2018

Choose a reason for hiding this comment

akirak Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akirak commented Feb 17, 2018

akirak commented Feb 18, 2018

akirak commented Feb 18, 2018

Golmote commented Feb 18, 2018

JuanCaicedo commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuanCaicedo commented Apr 5, 2018

Golmote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuanCaicedo commented Apr 5, 2018

Golmote commented Apr 5, 2018

JuanCaicedo commented Feb 16, 2018 •

edited

Loading

Golmote left a comment •

edited

Loading

akirak Feb 17, 2018 •

edited

Loading