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

Allow +nil+ for kramdown math_engine and syntax_highlighter options #644

Closed
wants to merge 0 commits into from

Conversation

mahrud
Copy link

@mahrud mahrud commented Jun 4, 2019

This allows users to explicitly turn off math engine and syntax highlighter of kramdown, following #545.
I also applied the changes requested by @parkr to prevent non-nil values.

@mahrud mahrud force-pushed the master branch 4 times, most recently from 2c8d018 to 4aa18b5 Compare June 4, 2019 21:53
@parkr
Copy link
Contributor

parkr commented Jun 5, 2019

This is great! I'd love some test coverage for this:

  1. Resulting configuration has nil value when set to nil
  2. Resulting configuration has mathjax value when unset
  3. Resulting configuration has mathjax value when set to unsupportedjax

Same set of tests for syntax_highlighter. This should be tested in spec/github-pages/configuration_spec.rb in the context "#effective_config" block, please!

@mahrud
Copy link
Author

mahrud commented Jun 6, 2019

Thanks for the suggestion! Turns out just accepting +nil+ wouldn't work, because it is treated the same as not having specified anything, so now instead I check for "nil". Incidentally, kramdown is okay with getting "nil" and treats it like you'd expect, but just to make the test more standard I change "nil" to +nil+ after the merge with defaults has happened.

@mahrud
Copy link
Author

mahrud commented Jun 13, 2019

@parkr is there anything else I could add?
The only alternative to using string "nil" to mean no mathjax was using something like false or "nomath" or some other constant, but "nil" seemed like the most reasonable one because kramdown gives an error if it can't find the math engine unless it is the string "nil".

@parkr
Copy link
Contributor

parkr commented Aug 16, 2019

I'd really much prefer that the user specifies nil and we explicitly check the user's config for this instead of the pre-merged values. I know we load things slightly different in GitHub Pages's code, but maybe we could go that route in both places to respect the true meaning of YAML.

@chrisyeh96
Copy link

Hi @parkr, are there updates on this pull request? I would love to see this merged in order to set math_engine: nil. Thanks!

@mahrud
Copy link
Author

mahrud commented Apr 15, 2020

@parkr I didn't understand your last comment at the time and forgot about this. I just rebased the PR to fix the conflicts. I don't think the test failing has anything to do with my changes though.

The problem, as I see it, is that giving nil for math_engine is treated the same as giving no value, which is interpreted as using the default, i.e. mathjax, so changing this behavior would break people's website. That's why checking for the string "nil" as an alternative seemed reasonable. Would checking for "" (empty string) be better?

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.

3 participants