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

[Markdown] Sync with MarkdownEditing 3.1.1 #3167

Merged

Conversation

deathaxe
Copy link
Collaborator

@deathaxe deathaxe commented Dec 11, 2021

Fixes #1930
Fixes #2542
Fixes #2857
Fixes #2942
Fixes #3073
Addresses #3154 (in ST3 compatible way)
Fixes #3157
Fixes #3170
Addresses #3228

This PR refactors Markdown syntax to

  1. improve compliance with CommonMark specification:
    https://spec.commonmark.org/0.30

    Many tests are added to proof compliance:
    https://spec.commonmark.org/0.30/spec.json

    Latest GFM syntax is based on CommonMark at the time writing,
    so those rules are included as well:
    https://github.github.com/gfm/

  2. convert most anonymous to named contexts for better support
    of inheritance.

  3. reorganize syntax into logical sections for better readability
    and maintainability.

  4. address most Markdown related issues of sublimehq/Packages repo.

Main changes are:

  • fix CommonMark compatibility of backslash escapes
  • fix CommonMark compatibility of block quotes
  • fix CommonMark compatibility of html entities
  • fix CommonMark compatibility of fenced code blocks
  • fix CommonMark compatibility of indented code blocks
    (mixed tabs/spaces)
  • fix CommonMark compatibility of reference definitions
  • fix CommonMark compatibility of thematic breaks
  • update strike-through markup to use 2 tildes only
  • reorganizing contexts in logical sections

Benchmarks:

This commit has no impact on parsing performance.

Notes:

  1. Some remaining CommonMark incompatibilities need further work
    using ST4's "branching" feature.

  2. Main refactoring work was done in MarkdownEditing package and
    released with version 3.1.1. This commit contains the result
    of that work, except some features which rely on 3rd-party
    (syntax) packages.

    Removed features are:

    • coffee script support in front-matter
    • numerous code-block syntaxes (E.g.: Ada, Coffee Script, ...)
    • LaTex blocks
    • custom <kbd> tag highlighting as it doesn't meet
      quality expectations

Fixes sublimehq#1930
Fixes sublimehq#2542
Fixes sublimehq#2857
Fixes sublimehq#3073
Fixes sublimehq#3154
Fixes sublimehq#3157

This commit proposes to apply refactored Markdown syntax from
MarkdownEditing, which was originally been based on ST's default
Markdown syntax.

It is mainly a reorganization of existing contexts/patterns, which
include several fixes which have been applied during refactoring.

It does not yet introduce branching to a broader extend in order to
limit changes to the syntax test file. Those are planned for future
PRs after being developed and tested in MarkdownEditing repo.

Note:

1. All 3rd-party syntaxes or extensions have been removed.
2. For details about single changes, please follow MarkdownEditing's
   commit history. All syntax related commits start with "Syntax: ".
@deathaxe deathaxe force-pushed the pr/markdown/sync-with-markdownediting branch from ebf80a8 to e2f597f Compare December 11, 2021 17:44
This commit reorganizes fenced syntax completions by package and adds
those which have been added by syncing with MarkdownEditing.
This commit renames some contexts to plural to express non-popping
behavior.
@jrappen
Copy link
Contributor

jrappen commented Jan 11, 2022

@deathaxe maybe mark this as significant?

@deathaxe
Copy link
Collaborator Author

The significant changes are not yet part of this PR ;-)

Fixes sublimehq#2942
Fixes sublimehq#3170

This commit proposes to apply refactored Markdown syntax from
MarkdownEditing 3.1.1, which was originally been based on ST's default
Markdown syntax.

This commit fixes various compatibility issues with CommonMark and
adds all related test cases of value to prove compliance from
https://spec.commonmark.org/0.30/spec.json.

Main changes are:

* fix regression with latex block highlighting in list items
* fix CommonMark compatibility of backslash escapes
* fix CommonMark compatibility of block quotes
* fix CommonMark compatibility of html entities
* fix CommonMark compatibility of fenced code blocks
* fix CommonMark compatibility of indented code blocks
  (mixed tabs/spaces)
* fix CommonMark compatibility of reference definitions
* fix CommonMark compatibility of thematic breaks
* update strikethough markup to use 2 tildes

It does not yet introduce branching to a broader extend in order to
limit changes to the syntax test file. Those are planned for future
PRs after being developed and tested in MarkdownEditing repo.

Note:

1. All 3rd-party syntaxes or extensions have been removed.
2. For details about single changes, please follow MarkdownEditing's
   commit history. All syntax related commits start with "Syntax: ".
@deathaxe deathaxe changed the title [Markdown] Sync with MarkdownEditing 3.1.0 [Markdown] Sync with MarkdownEditing 3.1.1 Jan 23, 2022
@jrappen
Copy link
Contributor

jrappen commented Jan 26, 2022

Did you want a review now, or will you add more stuff later?

@deathaxe
Copy link
Collaborator Author

I don't currently have anything valuable to add in the near future. The next milestone is to introduce sublime-syntax version 2 and rework emphasis (bold, ...) and other things using branching to further improve CommonMark compatibility. But this will take some time, I guess. Feedback about obvious issues would help at any point, of course.

The current syntax still suffers some limitations (e.g.: no multiline bold/italics in block quotes), but the solution I have in mind requires some effort based on the branching stuff.

@FichteFoll
Copy link
Collaborator

It is significant in the sense of effort required to review since the changes are huge, not in the effect.
(Alas, I'm skipping this one. Sorry folks 🤞 )

@michaelblyons
Copy link
Collaborator

I would be trying it out already, but it conflicts with the SQL change I need for work 😅 If that gets merged, I'll definitely run this next.

@deathaxe
Copy link
Collaborator Author

In which way does it conflict? I've checked out #3046 as well. The required dialect is set via the chooser plugin.

@michaelblyons
Copy link
Collaborator

michaelblyons commented Jan 27, 2022

I think it's some Markdown syntax test code, maybe with embedded SQL. I'll take a look.

Yeah, that was it, but the conflict was minimal. I have them both enabled, now.

@deathaxe
Copy link
Collaborator Author

This syntax definition is released to the public. So hopefully audience is large enough to get some feedback about issues.

Most remarkable changes are related with list blocks, block quotes and preventing indented code blocks from breaking nested lists. Stacking block quotes and lists is somewhat challanging.

It seems common practice to treat `shell` code blocks as interactive
shell. Means each line starting with `$` is treated as shell command,
while other lines denote output only, which may not be highlighted.
This commit ensures to start interactive mode not before a line
beginning with `$`. Continuation lines start with `>`.

Ineractive mode means each line starting with `$` is scoped shell,
while everything else is plain text - the output.
In case Shell-Unix-Generic.sublime-syntax is overwritten using another
syntax dialect it may happen `no_escape_behind` variable is not defined
by a dialect. Hence replace it to avoid future issues.
@jrappen
Copy link
Contributor

jrappen commented Mar 9, 2022

Maybe now would be a good time to move the test file to ./tests/** and split it up.

@deathaxe
Copy link
Collaborator Author

Wasted time.

@jrappen
Copy link
Contributor

jrappen commented Apr 2, 2022

@deathaxe please add above that (due to 2851722) this addresses #3228

@jrappen
Copy link
Contributor

jrappen commented Apr 5, 2022

From a quick once-over this seems fine, a review will take some time.

@jrappen
Copy link
Contributor

jrappen commented Apr 10, 2022

This PR seems fine after a manual review of the syntax files. I'd appreciate making all (?x)... explicit as (?x:...).

@deathaxe
Copy link
Collaborator Author

deathaxe commented Apr 11, 2022

While "explicit" (?x: ...) makes absolutely sense in variables as those may be combined in patterns, there's no technical need to enforce it in normal patterns.

I find the global (?x) style results in cleaner results when used in normal patterns. It avoids another line of code and another indentation level.

@deathaxe
Copy link
Collaborator Author

deathaxe commented May 6, 2022

FWIW No syntax related issue has been opened at https://github.com/SublimeText-Markdown/MarkdownEditing so far.

@keith-hall
Copy link
Collaborator

My main hesitation with reviewing (apart from the large diff) is the note in the OP making it sound like something was removed but not having concrete details:

Note:

  1. All 3rd-party syntaxes or extensions have been removed.

Can you elaborate a bit please? Extensions refers to file extensions, or Markdown syntax extensions like footnotes etc.?

@deathaxe
Copy link
Collaborator Author

deathaxe commented May 6, 2022

This comment refers to SublimeText-Markdown/MarkdownEditing@f4ef135 and doesn't mean any kind of step backward compared to what ST's Markdown supports at the moment.

MarkdownEditing supports ...

  1. way more syntaxes in fenced code-blocks than ST's default syntax - all of them sourced from 3rd-party packages, which we don't want to add dependencies for in this repo.

  2. frontmatter written in coffee script, also a 3rd-party syntax, which has been removed for same reason as (1.)

  3. LaTeX blocks ($$ ... $$) which has been removed as I am uncertain whether it is common enough to be supported by core Markdown.

  4. custom highlighting of <kbd>, which has been removed as it doesn't satisfy all the special cases elaborated in [Markdown] <kbd> tags deserve dedicated scope #2847.

It's basically a "normal" refactoring of current Markdown. I just decided to track all the little changes in MDE repo as it was where work started.

@jrappen
Copy link
Contributor

jrappen commented May 6, 2022

Thanks for the additional input.

Maybe you could move that info to the opening comment, i.e. commit message for the merge commit, before merging with master.

@deathaxe deathaxe merged commit b31d078 into sublimehq:master May 7, 2022
@deathaxe deathaxe deleted the pr/markdown/sync-with-markdownediting branch May 7, 2022 09:48
@jwortmann
Copy link
Contributor

LaTeX blocks ($ ... $ and $$ ... $$) are now supported by GitHub: https://github.blog/changelog/2022-05-19-render-mathematical-expressions-in-markdown/

Test: $x_{n+1} = rx_n(1-x_n)$

This might be a reason to add support for them into the core Markdown syntax via another PR.

@jrappen
Copy link
Contributor

jrappen commented May 19, 2022

... are now ...

open a new issue

@deathaxe
Copy link
Collaborator Author

deathaxe commented May 20, 2022

We already have one (see: #3251).

But yes, I agree we should just ignore any comment on closed PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment