Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Aug 23, 2021

Please, take some extra time reviewing this pull request as it is my very first code contribution to an open-source project. Any recommendation for improvements is very welcome.

  1. This is an experimental feature for providing an API for plugins. The plugin API might change, and certainly there is room for performance improvements.
  2. Comrak offers safe HTML escaping, however, it is not guaranteed that the plugins also generate a safe output. Thus it is the plugin developers' responsibility to take care of that.
  3. A sample plugin utilizing the syntect syntax highlighter is included, and syntect produces a safe HTML output.
  4. By default, the CLI tool uses the syntect plugin for highlighting, and it uses the 'base16-ocean.dark' theme of syntect. The theme can be changed using the --syntax-highlighting option, or the highlighting can be disabled by providing --syntax-highlighting none.
  5. The cm module currently is not using any plugin.
  6. These changes should not break those codes that are already using the comrak library as there are separate formatting functions that utilize the plugins, and the original "pluginless" functions should work the same as before.

@kivikakk
Copy link
Owner

Hi, Daniel! Thank you so much for your submission. I am going to be a little busy in the earlier half of this week, but as soon as I get a chance, I will review this thoroughly.

I will say now, though, this looks fantastic, and I'm excited to work with you to land these changes! ❤️

@ghost
Copy link
Author

ghost commented Aug 23, 2021

Thank you! In the meantime, I'll look into these build failures.

@ghost
Copy link
Author

ghost commented Aug 28, 2021

In #166 you've said,

We could add the functionality behind a feature flag and make it only default for the binary

Are you sure, syntax highlighting should be enabled for the binary by default? Because if I'm not mistaken, the reason why the build is failing is that I've made syntax highlighting enabled by default for the CLI.

@kivikakk
Copy link
Owner

Hiya! Sorry I haven't been around, I have been dealing with some health issues.

I do think syntax highlighting should be enabled by default for the binary, although I also think we should make that depend on a syntect feature (enabled by default).

As for CI, I think we should just alter CI to run the spec tests with syntax highlighting disabled! (i.e. pass --syntax-highlighting none in the harness scripts.) How does that sound to you?

@ghost
Copy link
Author

ghost commented Aug 29, 2021

Hi! No problem, I've been busy lately too.

I've been thinking about altering the tests as well, but then in my opinion, each currently failing test case should have three versions: one with no syntax highlighting, and one with syntax highlighting by no extra CLI arguments (i.e. the default syntax highlighting), and one with choosing a different theme via CLI arguments.

But these kind of tests wouldn't be very robust, because what if the color themes change over time? Since there are color codes in the output, that would mean, at least 82 (or 164, if both themes change) test cases should be altered just to update color codes, which sounds a bit tedious (although we can expect that themes wouldn't change too often within syntect).

Should we go for no syntax highlighting only, or test syntax highlighting as well?

Thank you, and wishing you all the best regarding the health issues!

Daniel Simon and others added 3 commits August 29, 2021 15:01
syntect is optional in the library (but defaults to on), and required in
the binary until someone really wants a syntect-less comrak binary for
some reason.

Add CI to ensure building and testing without these default features
does in fact work.
@kivikakk
Copy link
Owner

I think we are fine to not rigorously test syntect's output, and trust that syntect do that well enough themselves; we leave the specs as they are, pure CommonMark specs, and then the syntect_plugin test you have added is sufficient to ensure that the glue code is working, and not too brittle; in the unlikely event base16-ocean.dark does change, it's a very small update for us. I think the coverage you have in place now is just right.

I'm aware you have asked for some extra care in seeing this through, but I have reviewed it thoroughly and I couldn't complain if I tried! I have made two small changes; a typo fix in 8468c60, and making syntect feature (de)selectable in a6886b7. I also add a CI job for building and testing the library with features deselected there.

This is good to go. Thank you very much! ❤️

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.

1 participant