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

Rcfile options and their application to test output updates #499

Merged
merged 2 commits into from Mar 7, 2018

Conversation

@ccorn
Copy link
Contributor

@ccorn ccorn commented Mar 4, 2018

This short sequence of commits introduces

  1. New CLI-only options -n, --no-rcfile and -r FILE, --rcfile FILE that control whether to load a kramdownrc file as well as its pathname. The -n ensures that no kramdownrc can influence the output of your invocation of bin/kramdown, and the -r FILE replaces the default configuration with that contained in FILE. With those, generating test reference outputs from the command line can be done as follows:

    ruby -Ilib bin/kramdown -r mytest.options mytest.text >mytest.html

    Note that mytest.options does not need to exist. As kramdownrc is optional, so is the file named with -r.

  2. An update to Rakefile that makes use of those options. The existing targets dev:test_*_deps and dev:update_*_tests have made use of bin/kramdown before; those invocations are now protected from kramdownrc influence and look more like the example above. Also, instead of manually enumerating test file names in the Rakefile, globs are used now.

  3. New Rakefile targets for the MathjaxNode engine similar to those for SsKaTeX and KaTeX. Concretely, a new target dev:update_mathjaxnode_tests and its dependency dev:test_mathjaxnode_deps which does some sanity checking as to whether MathjaxNode actually works.

With those, the recent MathjaxNode test reference output updates could have been accomplished with the command

rake dev:update_mathjaxnode_tests

And the output (if a working mathjax-node-cli is installed) would be

NO RELEASE NOTES/CHANGES FILE
MathjaxNode is available, and its default configuration works.
/home/ccorn/.rvm/rubies/ruby-2.4.2/bin/ruby -Ilib bin/kramdown -r test/testcases/span/math/mathjaxnode.options test/testcases/span/math/mathjaxnode.text >test/testcases/span/math/mathjaxnode.html.19
/home/ccorn/.rvm/rubies/ruby-2.4.2/bin/ruby -Ilib bin/kramdown -r test/testcases/block/15_math/mathjaxnode_notexhints.options test/testcases/block/15_math/mathjaxnode_notexhints.text >test/testcases/block/15_math/mathjaxnode_notexhints.html.19
/home/ccorn/.rvm/rubies/ruby-2.4.2/bin/ruby -Ilib bin/kramdown -r test/testcases/block/15_math/mathjaxnode.options test/testcases/block/15_math/mathjaxnode.text >test/testcases/block/15_math/mathjaxnode.html.19
/home/ccorn/.rvm/rubies/ruby-2.4.2/bin/ruby -Ilib bin/kramdown -r test/testcases/block/15_math/mathjaxnode_semantics.options test/testcases/block/15_math/mathjaxnode_semantics.text >test/testcases/block/15_math/mathjaxnode_semantics.html.19
Copy link
Owner

@gettalong gettalong left a comment

Thanks for your contribution!

This should then be two commits, one for the config file related changes and one for the Rakefile adjustments.

Loading

bin/kramdown Outdated
@@ -46,6 +42,9 @@ OptionParser.new do |opts|
"html, GFM or markdown") {|v| options[:input] = v}
opts.on("-o", "--output ARG", Array, "Specify one or more output formats separated by commas: " \
"html (default),", "kramdown, latex, pdf, man or remove_html_tags") {|v| format = v}
opts.on("-n", "--no-rcfile", "Do not read any configuration file") {config_file = nil}
opts.on("-r", "--rcfile FILE", "Read specified configuration file, if it exists.",
"Default: #{config_file}") {|v| config_file = v}

Copy link
Owner

@gettalong gettalong Mar 6, 2018

Choose a reason for hiding this comment

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

I would go with --config-file FILE and --no-config-file, no short codes. And mention in --no-config-file that the default is using it if it is available. Should also go into the man page.

Loading

bin/kramdown Outdated
@@ -79,6 +78,17 @@ OptionParser.new do |opts|
end.parse!

begin
if config_file && File.exist?(config_file)
rcopts = YAML.safe_load(File.read(config_file), [Symbol])
Copy link
Owner

@gettalong gettalong Mar 6, 2018

Choose a reason for hiding this comment

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

Please use config_file_options or something similar instead of rcopts.

Loading

ccorn added 2 commits Mar 6, 2018
man/man1/kramdown.1.erb: Updated manpage source accordingly.

This is to improve control of configuration file usage.

Empty configuration files are now considered valid.
- Globbing for testcases files instead of hardcoding filename lists.
- Using the executable's `--[no-]config-file` options for better control.
- New target `dev:update_mathjaxnode_tests` to update MathjaxNode test
  reference outputs, and `dev:test_mathjaxnode_deps` for sanity checks.
@ccorn
Copy link
Contributor Author

@ccorn ccorn commented Mar 6, 2018

Done as requested. Notes:

  • I have taken the liberty to add a little more explanation of the option naming in the --help output and in the manpage. The explanations are worded differently because the manpage has some useful introduction that the --help output has not.
  • Without short option names, the --help output looked as if --no-config-file somehow belonged to the description of -o. I have added separator gaps to avoid that impression. If you find that still out of balance, consider adding three more separator gaps to separate all option descriptions without exception, as in the manpage.
  • Besides: The -o description is a bit too wide for an 80-column terminal; breaking the line after commas: would look better. Not included here because it's off-topic.
  • Changed the comments for the nil YAML case to be more precise. I should mention that the treatment of nil is a change to previous behavior; the rationale being that empty configuration files should be considered valid.

Loading

@ccorn
Copy link
Contributor Author

@ccorn ccorn commented Mar 6, 2018

Just noticed that the revised commit dates are wrong; seems to be an artifact of rebasing. I assure you that I have not foreseen your requests two days earlier :-)

Loading

@gettalong gettalong merged commit 30ea80d into gettalong:master Mar 7, 2018
1 check passed
Loading
@gettalong
Copy link
Owner

@gettalong gettalong commented Mar 7, 2018

Thank you for your changes!

Loading

@ccorn ccorn deleted the rcfile branch Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants