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

Highlight Elixir string markdown header correctly #775

Merged
merged 1 commit into from
Sep 22, 2015

Conversation

danielberkompas
Copy link
Contributor

Elixir allows markdown inside its triple-quote heredoc syntax. However,
the "#" used in markdown headers is currently misinterpreted as a
comment. So, if you write this:

@doc """
Get the first name of a user.

## Parameters

- `user` - A User struct.

## Examples

    user = %User{name: "Alice Winston"}
    User.first_name(user)
    "Alice"

"""

Prism interprets the markdown headers as comments, not as part of the
string, and this messes up the coloring.

04__functions__modules__and_structs___learnelixir_tv

It's easy to fix, just put the string regular expressions before the
comment regular expression in the elixir component.

@Golmote
Copy link
Contributor

Golmote commented Sep 22, 2015

Thanks for contributing!
Unfortunately, this change will lead to other issues (e.g. strings will be highlighted inside comments (breaking them))...
Comments might be used to comment code out, and that's why we usually handle them first.

Would it be ok to only change the negative look-ahead in the comment pattern, so that we ignore comments starting with more than one hash? /#(?![{#]).*/

@danielberkompas
Copy link
Contributor Author

I'll give it a try and let you know.

@danielberkompas danielberkompas force-pushed the fix-elixir-markdown-docs branch 2 times, most recently from daa8ac3 to 1096ccb Compare September 22, 2015 20:04
@danielberkompas
Copy link
Contributor Author

Changing the negative lookahead wasn't enough. If it was given this:

@doc """
## Header
"""

It would match on # Header. Since Javascript doesn't support negative lookbehinds, I added a negated capture group to assert that the # is not preceded by any other #s. What do you think of this solution?

@Golmote
Copy link
Contributor

Golmote commented Sep 22, 2015

Damn you're perfectly right! Please use the lookbehind feature provided by Prism, though, so that the char before the hash is not consumed:

'comment': {
    pattern: /([^#])#(?![{#]).*/,
    lookbehind: true
}

Can you also fix the indentation before the comments you added?

I can merge this once it's done ;)

Elixir allows markdown inside its triple-quote heredoc syntax. However,
the "#" used in markdown headers is currently misinterpreted as a
comment. So, if you write this:

```elixir
@doc """
Get the first name of a user.

- `user` - A User struct.

    user = %User{name: "Alice Winston"}
    User.first_name(user)
    "Alice"

"""
```

Prism interprets the markdown headers as comments, not as part of the
string, and this messes up the coloring.

This can be fixed by adding a negative lookbehind, such that the
comments regex matches "#" but not "##".
@danielberkompas
Copy link
Contributor Author

Done! I amended the commit.

@Golmote
Copy link
Contributor

Golmote commented Sep 22, 2015

Thanks!

Golmote added a commit that referenced this pull request Sep 22, 2015
Highlight Elixir string markdown header correctly
@Golmote Golmote merged commit 2e637f0 into PrismJS:gh-pages Sep 22, 2015
@danielberkompas danielberkompas deleted the fix-elixir-markdown-docs branch September 22, 2015 21:20
@Golmote
Copy link
Contributor

Golmote commented Sep 22, 2015

Oh, I had to add an alternative to handle the empty comment # and pass the tests.
The final pattern is: /(^|[^#])#(?![{#]).*/m

@danielberkompas
Copy link
Contributor Author

How do you run the tests?

@Golmote
Copy link
Contributor

Golmote commented Sep 22, 2015

By running npm test (see http://prismjs.com/test-suite.html)

@danielberkompas danielberkompas restored the fix-elixir-markdown-docs branch September 22, 2015 21:35
@danielberkompas
Copy link
Contributor Author

I'm fixing this now.

@Golmote
Copy link
Contributor

Golmote commented Sep 22, 2015

It's fixed already! See ccb6566

@danielberkompas
Copy link
Contributor Author

👍

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.

2 participants