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

JSON Language #370

Merged
merged 17 commits into from
Dec 30, 2015
Merged

JSON Language #370

merged 17 commits into from
Dec 30, 2015

Conversation

CupOfTea696
Copy link
Contributor

Added JSON support to prism.

@CupOfTea696
Copy link
Contributor Author

Alternatively, 'variable' could be used instead of 'property', which would look nicer in my opinion but is semantically incorrect.

@LeaVerou
Copy link
Member

See my comments on #369, they remain valid.
Btw you can update pull requests, you don’t need to close them and send a new one. :)

@CupOfTea696
Copy link
Contributor Author

Right, sorry I didn't know you could do that.
First of, the reason I added JSON is because the highlighting didn't work properly when just using javascript, I got a TypeError on var match = pattern.exec(str);
The reason I didn't extend javascript is because javascript has a lot more things in it than JSON has, so they would have to be removed from the definition and I wasn't sure that was possible. Regardless, it doesn't seem to make sense to me if you extend something and than scratch at least 50% of the parent.
Secondly, I hadn't realised that single quotes strings weren't allowed in JSON, so I do need yo update that.
Lastly, the reason why I didn't just use the i flag for the booleans and null is because things like faLSE aren't semantically correct, although it probably would make sense to highlight them anyway.

Also I'm still debating on wether I should use 'property' or 'variable' for the properties. It might be a good idea to just use property and then add some css for .language-json .token.property

Suggested by owner in #369
proper JSONP support.
@apfelbox
Copy link
Contributor

Lastly, the reason why I didn't just use the i flag for the booleans and null is because things like faLSE aren't semantically correct, although it probably would make sense to highlight them anyway.

Neither is NULL. So you should either include all cases or just the one in the spec (null).

Also I'm still debating on wether I should use 'property' or 'variable' for the properties. It might be a good idea to just use property and then add some css for .language-json .token.property

Or define it as alias (@LeaVerou did we agree on using alias for things like this)?

For the implementation: I would probably go with extending javascript, too. At least for not having to duplicate a lot of code.
Prism is no lint tool - so accepting invalid JSON is perfectly fine here.

If there are issues with using javascript with JSON data: all right - let's fix it. 😃

@apfelbox apfelbox mentioned this pull request Sep 22, 2014
@uranusjr
Copy link
Contributor

There is actually one extreme edge case, as described in this post, in which the JavaScript rules wouldn’t highlight JSON properly, while this implementation does. This can be fixed by extending JavaScript instead, obviously, but that seems a bit backwards IMO to include so much (C-like and JavaScript) for so few syntax features.

@CupOfTea696
Copy link
Contributor Author

If you do want me to move the JSON to a javascript extension, you'd have to tell me how to remove the things it inherits that it doesn't need. But I completely agree with @uranusjr on this one. I just checked and JSON would only inherit 2 definitions (number and boolean), which really doesn't seem to make it worthwhile.

@CupOfTea696
Copy link
Contributor Author

By the way, is there a way I can safely rename my branch, because I get a build warning email with every commit because the branch is still named gh-pages.

@LeaVerou
Copy link
Member

By the way, is there a way I can safely rename my branch, because I get a build warning email with every commit because the branch is still named gh-pages.

That’s a known issue that will eventually be solved once we separate master and gh-pages… That would also satisfy the bower crowd. The problem is that currently, I'm not sure how to make such a split in a DRY manner.

@LeaVerou
Copy link
Member

If you do want me to move the JSON to a javascript extension, you'd have to tell me how to remove the things it inherits that it doesn't need.

delete Prism.languages.foo;

I just checked and JSON would only inherit 2 definitions (number and boolean), which really doesn't seem to make it worthwhile.

You’re right, it’s not worth it just for two tokens, one of which is rather simple. @apfelbox what do you think?

Also, IMO we should merge JSON and JSONP, either by having both on the same file, or in the same language definition too (any opinions?)

@CupOfTea696
Copy link
Contributor Author

Same file would make sense, but I would recommend keeping the distinction between the two. Awaiting your decision before making additional changes.

@apfelbox
Copy link
Contributor

I would argue that loading too much doesn't harm a lot. The complete javascript markup is < 1kB, compile time in the browser is practically non-existant.

The only difference between JS and JSON seems to be (according to the link @uranusjr posted) native support for UTF-8 in all strings - which we could easily fix by just allowing them in JS too (again: Prism is a forgiving highlighter, no merciless linter).

But:
As my main issue (code duplication) doesn't stand (as @CupOfTea696 noted) I would vote for just releasing it as a standalone JSON + JSONP language. 👍

@apfelbox
Copy link
Contributor

(as a single language definition json with alias for jsonp)

@LeaVerou
Copy link
Member

Prism is a forgiving highlighter, no merciless linter

Excellent point. This should be in all bold on the contributor readme I think :p

I think the main issue between sharing strings in these two defs is that JSON doesn't allow single quoted strings (which also makes its regex easier, as no backreference is needed)

@CupOfTea696
Copy link
Contributor Author

Prism is a forgiving highlighter, no merciless linter

Since we are being so forgiving I really can't deny that putting the jsonp in the json definition and just making it an alias is a bad idea :)

JSONP alias added to JSON.
'null': /\bnull\b/gi,
};

Prism.languages.jsonp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prism.languages.jsonp = Prism.languages.json;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wow im stoopid :p

@AViscatanius
Copy link

Rename a branch? I hope it's easy

@LeaVerou
Copy link
Member

What happened here? Why did this stall?

@chris-martin
Copy link

JSON is the only thing I'm using Prism for, so I'm not sure whether this is a bug in this plugin or in the library, but FWIW: This doesn't work well with strings that contain numbers.

json

@Golmote
Copy link
Contributor

Golmote commented May 26, 2015

@chris-martin: This component has not made it into Prism yet. I think you'd better use the JS component to highlight your JSON, for now, as it seems to work properly:

http://puu.sh/i1Dkb/74db9493b2.png

@LeaVerou
Copy link
Member

@CupOfTea696 Reversing the order of the number and string tokens shall fix this. Please refer to the JS language definition for the optimal order.

@CupOfTea696
Copy link
Contributor Author

That should've fixed it.

@chris-martin
Copy link

I'm confused about how this can make a difference. Aren't JS objects unordered?

@Golmote
Copy link
Contributor

Golmote commented May 27, 2015

In theory they are. In practice all engines follow the order in which the properties were defined. And Prism relies on this behaviour.

@uranusjr
Copy link
Contributor

(See also discussion in #441)

@vkbansal vkbansal mentioned this pull request Jun 23, 2015
@markhalliwell
Copy link

This works rather well.

The only thing that I see missing is that if you use the show-language plugin, it only capitalizes the first letter. This will need to be added to the Languages object in https://github.com/PrismJS/prism/blob/gh-pages/plugins/show-language/prism-show-language.js#L8

Here is a temporary work around:

  Prism.hooks.add('before-highlight', function(env) {
    var pre = env.element.parentNode;
    if (!pre || !/pre/i.test(pre.nodeName)) return;
    if (/^json/i.test(env.language)) {
      pre.setAttribute('data-language', 'JSON');
    }
  });

@nauzilus
Copy link
Contributor

This is done automatically via the gulp build task: https://github.com/PrismJS/prism/blob/gh-pages/gulpfile.js#L52-L89

@darobin
Copy link
Contributor

darobin commented Nov 5, 2015

Is there any hope of this landing soonish? Highlighting with JS isn't very helpful as pretty much everything ends up being pretty much the same colour :) Also, I've been thinking of making it possible to style keys starting with @ differently so it works for JSON-LD too, if that's acceptable.

@Golmote
Copy link
Contributor

Golmote commented Nov 5, 2015

@darobin I'll try to make a proper review of this PR by the end of the week.

@adalinesimonian
Copy link

Hate to be that guy (I understand this is an OSS project), but any updates on a code review? This PR has been around for over a year now.

@drodil
Copy link

drodil commented Dec 23, 2015

Anything new to this?

inside: {
punctuation: /\(/
}
},
Copy link
Member

Choose a reason for hiding this comment

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

JSON doesn't have functions…

@LeaVerou
Copy link
Member

Sorry this has taken so long. I just took a look and added a few comments.

@CupOfTea696
Copy link
Contributor Author

@LeaVerou no worries! Applied the necessary changes.

LeaVerou added a commit that referenced this pull request Dec 30, 2015
@LeaVerou LeaVerou merged commit 106c817 into PrismJS:gh-pages Dec 30, 2015
@LeaVerou
Copy link
Member

Merged, thanks!
@darobin I think a change to accommodate JSON-LD would be awesome!! Please submit a PR for it, I would love this.

@drodil
Copy link

drodil commented Jan 4, 2016

Hi, good work! Is this going to be released anytime soon?

@LeaVerou
Copy link
Member

LeaVerou commented Jan 5, 2016

@drodil Any merged PR is immediately available to the live site.

Btw, @CupOfTea696, I noticed you made this enabled by default. While I did merge it to include it in Prism, I do not agree with it being enabled by default. Could you or someone else please send another PR to remove option: default?

@zeitgeist87
Copy link
Collaborator

Could you or someone else please send another PR to remove option: default?

I've removed the default option.

@LeaVerou
Copy link
Member

LeaVerou commented Jan 5, 2016

Thank you Andrea!!

@CupOfTea696
Copy link
Contributor Author

@LeaVerou That's probably because I copied it over from javascript. Sorry about that. Probably wasn't sure on what it did and just left it that way :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.