Skip to content

fix: Allow for pandoc style codefences#2468

Closed
dmadisetti wants to merge 3 commits intofacelessuser:mainfrom
AnotherGroupChat:dm/pandoc-codeblocks
Closed

fix: Allow for pandoc style codefences#2468
dmadisetti wants to merge 3 commits intofacelessuser:mainfrom
AnotherGroupChat:dm/pandoc-codeblocks

Conversation

@dmadisetti
Copy link

Great extension! Love its ubiquity. Noticed that the codefence logic does not match something like:

```haskell {.numberLines}
qsort [] = []
```

(see https://regex101.com/r/luUSyy/1)

Which is acceptable and expected in pandoc: https://pandoc.org/chunkedhtml-demo/8.5-verbatim-code-blocks.html#extension-fenced_code_attributes

+ fixes this and allows for a bit more flexibility in the attribute ordering: https://regex101.com/r/jEZuZ7/1

@gir-bot gir-bot added S: needs-review Needs to be reviewed and/or approved. C: source Related to source code. C: superfences Related to the superfences extension. labels Sep 26, 2024
@facelessuser
Copy link
Owner

  1. Our aim is not to match the accepted syntax of all parses. Our goal is not to match Pandoc in this way.

  2. The PR as written does not do what is intended either. It may not fail with haskell {.numberLines} is given, but it does not add the numberLines class as specified. Accepting the syntax and parsing the syntax are two separate things.

  3. This PR also allows other oddities which we do not want. For instance:

    ```{.haskell} {.numberLines}
    qsort [] = []
    ```

Unfortunately, I cannot accept this PR.

@dmadisetti
Copy link
Author

dmadisetti commented Sep 26, 2024

Maybe a bad example. The behavior between this pr and the current case is the same for

```{.haskell} {.numberLines}
qsort [] = []
```

since attributes match is greedy
Both match: attrs=".haskell} {.numberLines"


This might break fewer cases than you think, just allows different ordering for attributes

@facelessuser
Copy link
Owner

Both match: attrs=".haskell} {.numberLines"

I'm not sure how that addresses my three statements. Using the case I showed does not add any of these attributes.

In addition to my three earlier statements, it appears this has not yet been tested as it:

  1. Breaks at least one test.
  2. The cases it claims to allow, while passing through the parser, is not processed properly as attributes or options.

@dmadisetti
Copy link
Author

  1. Verified that existing tests do not break
  2. changed the regex to preserve attributes and options being exclusive
  3. Handle explicit lang declaration
  4. added additional test

I understand that there does not have to be parity, but it can't hurt (:

@gir-bot gir-bot added the C: tests Related to testing. label Sep 26, 2024
@facelessuser
Copy link
Owner

I am actually phasing out that old style of testing. SuperFence tests should go here: https://github.com/facelessuser/pymdown-extensions/blob/main/tests/test_extensions/test_superfences.py.

This doesn't mean I've decided to add this functionality, but I will at least consider it if you move the tests and ensure it is all working. Please make sure you've considered edge cases.

I'll think about this and give a response once I've decided.

if m is not None:

# Parse options
self.lang = m.group('lang') or ''
Copy link
Owner

Choose a reason for hiding this comment

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

I understand this was moved out to be in one place, but I would prefer that the language handling is parsed in the handle_attrs and parse_options functions.

@facelessuser
Copy link
Owner

I do think I'm okay with the idea. It seems like the changes will be minimal.

  1. Update to new style tests.
  2. Keep option/attribute handling in their respective functions.
  3. Ensure lint and tests are working, and I think I'm okay with it.

facelessuser added a commit that referenced this pull request Sep 27, 2024
@facelessuser
Copy link
Owner

Hey, @dmadisetti as I've warmed up to this idea, I was digging in and decided to clean up the regex and such, so I've issued a new PR that cleans up my requests and references you and this PR. That PR will supersede this one.

I think when it became apparent that we could do this with very little impact, it kind of became hard to argue against this. Admittedly, I think many will find this new format preferable.

Thanks for getting this rolling and pushing for it, I do think it'll be a good addition.

@dmadisetti
Copy link
Author

Much appreciated! Looking forward to the update

@dmadisetti dmadisetti closed this Sep 27, 2024
facelessuser added a commit that referenced this pull request Sep 27, 2024
 Support alternate fenced header form

Based off of @dmadisetti's code in PR #2468
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C: source Related to source code. C: superfences Related to the superfences extension. C: tests Related to testing. S: needs-review Needs to be reviewed and/or approved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants