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

Fix bug: handle "|" in $$..$$ or <code>..</code> #23

Closed
wants to merge 2 commits into from

Conversation

@tomtung
Copy link

@tomtung tomtung commented Oct 17, 2012

Given the following input kramdown input:

Codeblock:

<code>code|block</code>

Inline code: <code>some|code</code>

Code in table:

| <code>this | is</code> | <code>a | test</code>|

Formula:

$$ P(x) = \sum_z P(x|z)P(z) $$

Inline formula: $$ P(x) = \sum_z P(x|z)P(z) $$

Formula in table:

| $$P(x|y)$$ | $$P(y|z)$$|

Previously, kramdown would give the wrong output:

<p>Codeblock:</p>

<table>
  <tbody>
    <tr>
      <td><code>code|block</code></td>
    </tr>
  </tbody>
</table>

<table>
  <tbody>
    <tr>
      <td>Inline code: <code>some|code</code></td>
    </tr>
  </tbody>
</table>

<p>Code in table:</p>

<table>
  <tbody>
    <tr>
      <td><code>this | is</code></td>
      <td><code>a | test</code></td>
    </tr>
  </tbody>
</table>

<p>Formula:</p>

<table>
  <tbody>
    <tr>
      <td>$$ P(x) = \sum_z P(x</td>
      <td>z)P(z) $$</td>
    </tr>
  </tbody>
</table>

<table>
  <tbody>
    <tr>
      <td>Inline formula: $$ P(x) = \sum_z P(x</td>
      <td>z)P(z) $$</td>
    </tr>
  </tbody>
</table>

<p>Formula in table:</p>

<table>
  <tbody>
    <tr>
      <td>$$P(x</td>
      <td>y)$$</td>
      <td>$$P(y</td>
      <td>z)$$</td>
    </tr>
  </tbody>
</table>

After the bugfix, the output is:

<p>Codeblock:</p>

<p><code>code|block</code></p>

<p>Inline code: <code>some|code</code></p>

<p>Code in table:</p>

<table>
  <tbody>
    <tr>
      <td><code>this | is</code></td>
      <td><code>a | test</code></td>
    </tr>
  </tbody>
</table>

<p>Formula:</p>

<script type="math/tex; mode=display"> P(x) = \sum_z P(x|z)P(z) </script>

<p>Inline formula: <script type="math/tex"> P(x) = \sum_z P(x|z)P(z) </script></p>

<p>Formula in table:</p>

<table>
  <tbody>
    <tr>
      <td><script type="math/tex">P(x|y)</script></td>
      <td><script type="math/tex">P(y|z)</script></td>
    </tr>
  </tbody>
</table>
@namtih58
Copy link

@namtih58 namtih58 commented Jan 18, 2013

+1

@ghost ghost assigned gettalong Jan 19, 2013
@gettalong
Copy link
Owner

@gettalong gettalong commented Jan 20, 2013

Thanks for the pull request but it does not pass the tests under Ruby 1.8 due to invalid regexp syntax. If you fix the errors when run under Ruby 1.8 I could merge it.

You can test by installing various Ruby versions via rvm and then use the benchmark/testing.sh script.

@aprescott
Copy link

@aprescott aprescott commented Apr 22, 2014

Can this be looked at again and considered for merging? Does kramdown still aim to support 1.8 despite it reaching end of life?

@gettalong gettalong added this to the 2.0.0 milestone Apr 23, 2014
@gettalong
Copy link
Owner

@gettalong gettalong commented Apr 23, 2014

For kramdown 1.x, support for Ruby 1.8 is still supported (many enterprise Linux systems have very old Ruby versions...). However, with kramdown 2.x support for 1.8 will be dropped (and probably for 1.9, too).

@gettalong
Copy link
Owner

@gettalong gettalong commented Jun 15, 2014

Thanks again for this pull request. However, since it does not resolve the problem in a backwards compatible way and there are still other issues with pipe symbols and tables (e.g. #135) that won't be fixed by this, I will recommend using the \vert LaTeX command instead of | since they are equal.

The first two examples of lines that should be paragraphs instead of tables have been fixed by 2896c32.

I haven't tested this but also note that this pull request might not solve the problem of statements like:

Inline formula: $$ P(x) = \sum_z P(x|z)P(z)   
which uses | is $$.

However, when using \vert instead of | this problem is also resolved.

@aprescott
Copy link

@aprescott aprescott commented Jun 15, 2014

I think closing this is disappointing. Placing the fix in a new major version release with no 1.8 support is plenty possible, considering that 1.8 MRI is receiving no new official patches, including security fixes. Just because this PR doesn't fix all pipe issues doesn't make it closable IMO.

@gettalong
Copy link
Owner

@gettalong gettalong commented Jun 16, 2014

The next major release will most probably have a change in the table processing code, requiring that a table line starts with a pipe. So this would be a non-issue then.

The problem is that this addresses just some bugs with the table parsing code and very specifically only with inline math statements. There would probably be many other necessary adjustments in regard to other parsing functionality. The fix of using \vert instead of | in inline math statements and \| in text will always work, the others just might and so it is better to tell the users this instead of "yeah, using | might work in some cases".

I also just don't have that much time currently to investigate, pull requests are surely welcome!

@aprescott
Copy link

@aprescott aprescott commented Jun 16, 2014

Personally I think a real bug minimisation is better than an imagined perfect bug eradication, but yes I understand what you're saying. Hopefully the "most probably" change makes it to the next major release, good to hear!

@TheChymera
Copy link

@TheChymera TheChymera commented Jul 23, 2014

Seems like this is a recurring issue especially for fans of conditional probability ^^.

I don't see how in the absence of anything else a fix addressing just some of the bugs is not worth merging - "because | would then just be hit and miss for the user". It already is hit and miss. Example: I happily used it in mathblocks and then whoop all my inline text got scrambled when I expected it to work likewise inline.

@gettalong
Copy link
Owner

@gettalong gettalong commented Jul 23, 2014

I get your message but this pull request doesn't solve the problem in a backwards-compatible way, so it can't be merged.

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.