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 code block indentation in lists #455

Merged

Conversation

momja
Copy link
Contributor

@momja momja commented Jul 2, 2022

This is a fix for issue #276. There's a failure in converting markdown lists that have fenced code blocks in them, because the indentation of the fenced code block is not accounted for unless syntax highlighting is enabled.

I also pulled out the code for substituting code blocks with a lexer into its own method so it is a little easier to read.

@Crozzers
Copy link
Contributor

Crozzers commented Jul 3, 2022

This creates issues for fenced code blocks with syntax highlighting. When a fenced code block is passed to _code_block_sub, it now gets to this section:

# with fenced code blocks, we should outdent to the start of the fence,
# not the first line of code
indent_count = len(match.group(1)) - len(match.group(1).lstrip())
leading_indent, codeblock = self._uniform_outdent_limit(codeblock, ' '*indent_count)

This section will remove the fenced code block's leading indent, as intended.
The code block then reaches this section:
if lexer_name and "highlightjs-lang" not in self.extras:
lexer = self._get_pygments_lexer(lexer_name)
if lexer:
return self._code_block_with_lexer_sub(codeblock, lexer, is_fenced_code_block)

Where the syntax highlighted fenced code block will get passed to the new _code_block_with_lexer_sub function. This function is unaware that the code block has already been un-indented and therefore will not add back that initial indentation.

The code block needs to have that initial indentation restored before it is returned.

@momja
Copy link
Contributor Author

momja commented Jul 3, 2022

Good catch, I can make a revision to fix this. And I’ll add a test for this case too. Thanks @Crozzers.

@Crozzers
Copy link
Contributor

Crozzers commented Jul 3, 2022

I think the fenced_code_blocks_issue426 test case already covers syntax highlighted fenced code blocks inside of lists.
If anything, you could merge the fenced_code_blocks_issue426 test case with the tests added in this PR into the same file, since they do both cover fenced code blocks inside of lists

@nicholasserra
Copy link
Collaborator

@Crozzers any last opinions on this, besides the possibly dupe test?

@Crozzers
Copy link
Contributor

@nicholasserra No last opinions, looks good to me

@momja
Copy link
Contributor Author

momja commented Jul 12, 2022

I think it would help developers if it was more clear in testing that tests are being skipped when the pygments library is not installed. I thought all tests were passing until they were ran via the CI/CD pipeline. @Crozzers or @nicholasserra, what do you think would be a good option here? Could list tested passed, failed, and skipped. Or could show a warning that not al tests were run because certain modules were missing.

@Crozzers
Copy link
Contributor

For me, I don't have pygments installed on Python 3.8 and lower and when I run make test the following warning is printed:

-- test with Python 3.8 (/usr/local/bin/python3.8)
WARNING:test:skipping pygments tests ('pygments' module not found)

A similar warning is printed for each python version missing pygments.
However, given that these warnings are printed at the START of the tests, before being pushed off screen by a massive block of scrolling test results, I wasn't aware that this warning had been printed and didn't know why the Python 3.8 tests ran ~158 tests and Python 3.9 ran ~170 test cases.

I agree with @momja. I think perhaps changing when the warning is printed would help, maybe if it were printed at the end of the long scrolling block of test results of each python version?
I also think at the very end of the tests (after ALL python versions have been tested) it should re-print any warnings to make sure the person running the tests is fully aware of them.

@nicholasserra
Copy link
Collaborator

Tracking test warnings in #458

@nicholasserra
Copy link
Collaborator

Thank you for this patch @momja

@nicholasserra nicholasserra merged commit d2907be into trentm:master Jul 14, 2022
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.

None yet

3 participants