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

Improve single line comment formatting in files with CRLF #723

Closed
wants to merge 1 commit into from

Conversation

instantdelay
Copy link

I'm using Rouge through Jekyll to format some code snippets and I noticed that all my single line comments had an extra line after them in the formatted HTML. It looks like this is caused by the single line comment rule is some languages not handling windows line endings.

The rule for single line comments used $ to match newline but not capture it.
If a file is using CRLF this means the CR gets included in the comment value
but not the LF. This shows up as an extra line in the formatted HTML output
because one is inside the span and one is after.

Change to include anything up to the first CR or LF in the following languages:
- actionscript
- csharp
- groovy
- javascript
@dblessing
Copy link
Collaborator

@instantdelay Thanks for your contribution. As far as I know, Jekyll still relies on Rouge < 2.0. Unless they upgrade you won't be able to take advantage of your fix.

It'd also be nice if we could put a visual sample in to 'test' this change, but it's probably difficult to get a CRLF in there and not have it be fragile.

@instantdelay
Copy link
Author

Should I work adding a visual sample to this change?

For my purposes I edited my website's Gemfile to use
gem "rouge", :path => "../rouge"
where I have a copy of the source with my fix.

@gfx
Copy link
Member

gfx commented Aug 21, 2017

This PR is useful for Windows users but also is difficult to maintain it because I (and, I guess, so as other maintainers) uses non-Windows.

Rouge should normalize newlines before passing text to lexers, so each lexer should only handle LF as newlines.

@marius-balteanu
Copy link

@instantdelay, thanks for the proposed fix. I've just used your fix for Ruby files and it works as expected. Also, I've proposed a test for this scenario, please see #1078. If the PR is accepted, I can contribute with the tests for your PR.

@pyrmont
Copy link
Contributor

pyrmont commented May 30, 2019

@instantdelay I'm sorry that the first comment in such a long time is about closing this PR but a similar request (#1078) has just been reviewed and since the conclusion was to close that, I thought I would update both at the same time.

After discussion with the other maintainers, @jneen and I are of the opinion that Rouge should continue treating \r as 'significant' whitespace. As a result, we will not be accepting this PR.

As I wrote in the other thread, our logic is that the consumer of Rouge is best placed to decide whether to normalise the source input or not. ('Consumer' here means the entity that incorporates and calls Rouge as distinct from the end-user. In your case, Jekyll.) This position is similar to our stance regarding encoding: it's the consumer's responsibility to provide us a UTF-8-compatible string.

I realise your original PR is from almost two years ago but my understanding is that Jekyll does now normalise source input (see here). Hopefully this means that this is less of an issue than it was at the time.

I mentioned this in the other thread but while I'm closing this PR, please feel free to continue discussion here if you had any questions. Thanks very much for preparing the request and I'm really sorry it took so long to make a decision on this.

@pyrmont pyrmont closed this May 30, 2019
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.

5 participants