Skip to content

Conversation

MustafaHaddara
Copy link

There's probably a better way to do this -- I feel like tables.py should import from inlinepatterns or something like that.

Tests passed, and I added 4 tests for tables. On the master branch, the first 3 pass. These commits get the 4th one working.

Resolves #436

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be named _row_has_unpaired_backticks, as this really has little to do with code.

@mitya57
Copy link
Collaborator

mitya57 commented Oct 22, 2015

In general the idea looks good to me 👍, though I have left some comments on the code style issues.

Travis fails on Coverage check on Python 3.2 because Coverage doesn't seem to support that version, should we maybe exclude that check from the Travis config?

@waylan
Copy link
Member

waylan commented Oct 22, 2015

Interesting. What about when multiple backticks are used to delimit a codespan? Might want to add to the test some more cells:

`` some | code ``
``` some | code ```
```` some | code ````
`` some ` | ` code ``
``` some ` | ` code ```
```` some ` | ` code ````

Note that each of the first three should result in the following HTML:

<code>some | code</code>

And the last three:

 <code>some ` | ` code</code>

Copy link
Member

Choose a reason for hiding this comment

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

According to the coverage report, this is never tested. Might want to add a test for it.

@waylan
Copy link
Member

waylan commented Oct 22, 2015

Travis fails on Coverage check on Python 3.2 because Coverage doesn't seem to support that version, should we maybe exclude that check from the Travis config?

Yes, Coverage 4.0 does not support Python 3.2. We could require Coverage 3.x, but I don't really think it matters much at this point.

@waylan
Copy link
Member

waylan commented Oct 22, 2015

@MustafaHaddara, @mitya57 already pointed out the specific issues to you, but the Flake8 test shows all those style/formatting related issues for you as well.

@MustafaHaddara
Copy link
Author

@waylan @mitya57 I've made the style fixes and updates to the test...but attempting to use multiple backticks to denote a code block is still broken.

@waylan
Copy link
Member

waylan commented Oct 22, 2015

@MustafaHaddara this looks like a good start. When the multiple backticks issue gets worked out, either @mitya57 or I will merge it. Thanks for the report and getting the work started on this.

Copy link
Member

Choose a reason for hiding this comment

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

The Flake8 test is indicating this has trailing whitespace.

@MustafaHaddara
Copy link
Author

@waylan @mitya57 updated to deal with the issue of multiple backticks. I changed the implementation strategy from the manual string parsing I initially started with and instead, I use inlinepatterns.BacktickPattern to find inline code blocks inside of a table cell.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't do this. Lets keep all inline parsing in the inline parsing step of the parser. If you want to use the regex for codespans to identify them, I understand, but when the table blockparser is done, the cell should still contain Markdown formatted inline text. If you don't do it that way, then other (inline) extensions could fail in unpredictable ways on table cells with codespans.

@MustafaHaddara
Copy link
Author

@waylan @mitya57 fixed as requested. All tests pass for me. Speaking of tests, I didn't notice there was already some tables tests in /tests/extensions/extra/ so I had created a new tables test in /tests/extensions/. Do you want me to remove the test file I added and move those test cases to the older file?

And would you like me to rebase and squash all of these commits into one commit to encompass the entire patch?

@waylan
Copy link
Member

waylan commented Oct 25, 2015

The tables extension is part of extra, so yes I would prefer the tests be under tests/extensions/extra/. And yes, it would make sense to squash all of the commits.

Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you're not using match.end(group) here?

Every thing else looks good to me.

@MustafaHaddara
Copy link
Author

@waylan In response to your point about match.end(group), that gives us the end of the group -- and we only have groups for the first code block delimiter, the text inside the code block, and the text after the code block. Asking for the end of a group won't help.

What we really want is the beginning of the text outside the code block, ie. match.start(group).

Please review, and if it is good with you I'll rebase into one commit.

@waylan
Copy link
Member

waylan commented Oct 25, 2015

Looks good. Clean up the Flake8 errors and rebase and I'll merge.

@MustafaHaddara
Copy link
Author

The Flake8 test didn't run, and I'm not sure why. It threw ERROR: InvocationError: '/home/travis/build/waylan/Python-Markdown/.tox/flake8/bin/flake8 /home/travis/build/waylan/Python-Markdown/markdown /home/travis/build/waylan/Python-Markdown/tests /home/travis/build/waylan/Python-Markdown/setup.py /home/travis/build/waylan/Python-Markdown/run-tests.py'

Is there a more detailed log I can look at somewhere?

@waylan
Copy link
Member

waylan commented Oct 25, 2015

The Travis Report (linked below) shows the details for each build. Just select the failing build and it will give you a full log. As per our earlier discussion, you can ignore the Python 3.2 build due to a version incompatibility with Coverage.

@MustafaHaddara
Copy link
Author

Rebased, all ready to go.

waylan added a commit that referenced this pull request Oct 26, 2015
@waylan waylan merged commit 2e4f1f7 into Python-Markdown:master Oct 26, 2015
@MustafaHaddara MustafaHaddara deleted the table-inline-code branch November 7, 2015 16:42
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.

3 participants