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.

Sign up for GitHub

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

Jump to bottom

Fix Kramdown parser crash #546

Merged
merged 1 commit into from
Feb 16, 2021
Merged

Fix Kramdown parser crash #546

merged 1 commit into from
Feb 16, 2021

Conversation

maxim-belkin
Copy link
Contributor

... by using GFM (GitHub-flavored Markdown) parser (kramdown-parser-gfm)
instead of the default one (kramdown).

The default one fails to produce an AST (Abstract Syntax Tree) when
there is no blank line before the line with the opening code fence.

Related:

Fixes: #543

@maxim-belkin
Fix Kramdown parser crash
f419f9c 
... by using GFM (GitHub-flavored Markdown) parser (`kramdown-parser-gfm`)
instead of the default one (`kramdown`).

The default one fails to produce an AST (Abstract Syntax Tree) when
there is no blank line before the line with the opening code fence.

Related:
 - gettalong/kramdown#530
 - Python-Markdown/markdown#807

Fixes: carpentries#543
@maxim-belkin maxim-belkin mentioned this pull request Feb 15, 2021
Closed
@fmichonneau fmichonneau requested a review from zkamvar February 16, 2021 11:03
zkamvar
zkamvar approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

Thank you for adding this. I didn't even realize kramdown had split off their gfm parser into a different bundle!

@zkamvar zkamvar merged commit ec29c6b into carpentries:gh-pages Feb 16, 2021
@maxim-belkin
Copy link
Contributor Author

Thanks for merging, Zhian.

@maxim-belkin
Copy link
Contributor Author

Zhian, again -- thanks for merging and for reviewing the PR. I've just noticed that you kept the default message -- Merge pull request #546... -- I'd recommend tweaking these messages (in the future) changing #546 to carpentries/styles#546 because what will happen when lessons sync their templates with this repository is that all issues (or PRs) with number 546 in their repositories will link to this commit and that might confuse contributors/maintainers.

@zkamvar
Copy link
Contributor

zkamvar commented Feb 19, 2021

Zhian, again -- thanks for merging and for reviewing the PR. I've just noticed that you kept the default message -- Merge pull request #546... -- I'd recommend tweaking these messages (in the future) changing #546 to carpentries/styles#546 because what will happen when lessons sync their templates with this repository is that all issues (or PRs) with number 546 in their repositories will link to this commit and that might confuse contributors/maintainers.

Thank you for pointing this out! I honestly didn't even realize it was an option via the github interface.

@maxim-belkin maxim-belkin deleted the fix543 branch March 17, 2021 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkamvar zkamvar zkamvar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lesson_check.py parser crash on edge syntax case
2 participants
@maxim-belkin @zkamvar