Lukasa / language-restructuredtext

A ReStructuredText syntax package for Atom
MIT License
27 stars 15 forks source link

Added math scopes to grammar #44 #45

Closed mangecoeur closed 8 years ago

mangecoeur commented 8 years ago

Added scopes but highlighting not working. In theory a basic math highlight should be included by including the #math-syntax patterns but for some reason this doesn't work - if you have a better understanding of grammars perhaps you can suggest a fix.

Lukasa commented 8 years ago

Hrm, I'm afraid I don't. We may want to leave this open for someone who does to add detail too.

Alhadis commented 8 years ago

Nooooooo, you're doing it all wrong. :<

You don't copy+paste from another language grammar to embed its highlighting inside another. You do this:

patterns: [include: "scope.name"]

So it should just be this:

# Math
{
    name: "source.embedded.latex"
    contentName: "markup.math.block.restructuredtext"
    patterns: [include: "text.tex"]
    begin: "^([ \\t]*)(\\.\\.)\\s(math)(::)\\s*$"
    end:   "^(?!\\s*$|\\1[ \\t]+\\S)"
    beginCaptures:
        2: name: "punctuation.definition.directive.restructuredtext"
        3: name: "support.directive.restructuredtext"
        4: name: "punctuation.separator.key-value.restructuredtext"
}

This does of course require a user have a TeX grammar installed (if they don't, it won't cause any harm: the highlighting just won't show up). If that's a concern, you can always add it as a package dependency in package.json (so Atom will install the language grammar too, if it hasn't been installed already):

"package-deps": [
    "language-latex"
]

General best practice is to let the user install optional dependencies if they're not going to need them, but that's a design design I'll leave to you. But the bottom line is you never try and embed chunks of unrelated languages inside another grammar... it's a one-way ticket to mess hell.

mangecoeur commented 8 years ago

Sorry :P I just mimicked the approach from language-pfm since that seemed to work ok. Will try to update PR with your suggestion and see if it works

On 12 Jul 2016, at 13:25, John Gardner notifications@github.com wrote:

Nooooooo, you're doing it all wrong. :<

You don't copy+paste from another language grammar to embed its highlighting inside another. You do this:

patterns: [include: "scope.name"] So it should just be this:

Math

{ name: "source.embedded.latex" contentName: "markup.math.block.restructuredtext" patterns: [include: "text.tex"] begin: "^([ \t])(..)\s(math)(::)\s$" end: "^(?!\s*$|\1[ \t]+\S)" beginCaptures: 2: name: "punctuation.definition.directive.restructuredtext" 3: name: "support.directive.restructuredtext" 4: name: "punctuation.separator.key-value.restructuredtext" } This does of course require a user have a TeX grammar installed (if they don't, it won't cause any harm: the highlighting just won't show up). If that's a concern, you can always add it as a package dependency in package.json (so Atom will install the language grammar too, if it hasn't been installed already):

"package-deps": [ "language-latex" ] General best practice is to let the user install optional dependencies if they're not going to need them, but that's a design design I'll leave to you. But the bottom line is you never try and embed chunks of unrelated languages inside another grammar... it's a one-way ticket to mess hell.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Lukasa/language-restructuredtext/pull/45#issuecomment-232031874, or mute the thread https://github.com/notifications/unsubscribe/AAtYVAbjYQPTBnAxTDqmq-gK1lVaA6Uyks5qU4fPgaJpZM4JDUTX.

Alhadis commented 8 years ago

Tip: You can also import certain blocks from another grammar by including the name:

patterns: [include: "scope.name#block-id"]

... where block-id is a named pattern-block defined in the target grammar's repository property. So, to embed reStructuredText's directive patterns inside another grammar, you'd go:

patterns: [include: "text.restructuredtext#directives"]

This can be helpful if importing an entire grammar is screwing with the rest of the document, and you only need a certain subset of patterns (such as math-highlighting, in this case).

mangecoeur commented 8 years ago

Updated the PR to rather include highlighting from Latex grammar. Still works with preview-inline. The remaining issue is that Latex only applies math highlighting inside begin{}...\end{} blocks and I'm not sure how to pick the patterns out from that block and apply them inside the :math: block

mangecoeur commented 8 years ago

Made changes according to @Alhadis comment - highlighting now works!

Alhadis commented 8 years ago

Just a reminder: users will need the LaTeX language package installed in order for the math highlighting to display. If they don't, well, it just looks like uncoloured source... no big deal. Just wanted to make doubly-sure you were aware of that, though. =)

Lukasa commented 8 years ago

@Alhadis In recognition of your great work on this repo over the last few months, and because you know substantially more than I do about Atom highlighting packages, I've just sent you an invite to be a repo collaborator. You don't need my oversight: feel free to merge whenever you're happy with something!

FWIW, this LGTM. =)

Alhadis commented 8 years ago

Thanks mate! =) Don't worry, I'm pretty good at being responsible, I'm happy to help manage PRs and issues and stuff.

And yeah, erh, I spend wayyy too much time working with language grammars, haha. I'm starting to feel it's an unhealthy obsession... thanks!

Lukasa commented 8 years ago

My pleasure! I have so much on my plate that this repo ends up way far down my priority list, so it's good to have people who can help take care of it.

Alhadis commented 8 years ago

I'm pretty sure the owner of the File-Icons package thought the same thing, heh. =)

I'll look into/merge this soon, I'm just juggling pull requests ATM.

For an unemployed prick, there's no way I should be this busy.

Lukasa commented 8 years ago

There's no rush. =)

Lukasa commented 8 years ago

Thanks @mangecoeur! :sparkles: :cake: :sparkles:

Alhadis commented 8 years ago

@Lukasa Heh, the package is still technically in an alpha state. The release version is at v0.16.0.

Think we should finally make that a v1.0.0? ;)

Lukasa commented 8 years ago

Heh, I don't really mind. =D Version numbering a package with no API is kinda a "gut feel" thing rather than anything else.

Alhadis commented 8 years ago

True, but seeing a major release chillin' at v0 for a perfectly functional package just seems... wrong, heh.

Cutting a release. :)

Lukasa commented 8 years ago

Sounds good to me then. =)