htdebeer / pandocomatic

Automate the use of pandoc
https://heerdebeer.org/Software/markdown/pandocomatic/
GNU General Public License v3.0
158 stars 14 forks source link

Minor: handle empty pandoc declarations in templates #98

Closed iandol closed 2 years ago

iandol commented 2 years ago

While helping a user who faced the recent psych error, they mentioned they also got an error when commenting out the paru filter that triggered the psych error in their pandocomatic.yaml. They were commenting out the filter in a list but leaving the filter: specifier in place:

  html:
    pandoc:
      from: markdown
      to: html5
      standalone: true
      number-sections: false
      section-divs: true
      mathjax: true
      filter: 
        #- filters/assimilateMetadata.rb #regularise metadata
      template: 'templates/custom.html'

This was causing a nil error:

:: Running: /Users/ian/.rbenv/shims/pandocomatic --debug mdata.md
::: An unexpected error has occurred. You can report this bug via https://github.com/htdebeer/pandocomatic/issues/new.
::: /Users/ian/.rbenv/versions/3.0.2/lib/ruby/gems/3.0.0/gems/pandocomatic-0.2.7.8/lib/pandocomatic/configuration.rb:666:in `is_local_path?': undefined method `start_with?' for nil:NilClass (NoMethodError)

To be clear, the error is on the user end, but just wanted to mention this so you can choose if you want to guard against this.

htdebeer commented 2 years ago

I'd expect an empty list of filters to work as well. The question I have: Is this valid YAML to denote an empty list property? I think you should write "filter: []" for that. Anyway, a better error message would be nice, or assign the default value to properties without a value, or both.

iandol commented 2 years ago

I use VSCode with the Red Hat YAML extension which purports to validate YAML doesn't raise any errors. I also used some online validators and they say it is OK: http://www.yamllint.com & https://jsonformatter.org/yaml-validator

...though each validator replaces the empty list, the first replaces with ~ and the second with null -- from the YAML spec I couldn't quite determine if there is a proper empty content specifier (~ is not in the special character list at least: YAMLSpec-indicator-characters )

htdebeer commented 2 years ago

I tried a simple example:

require "pp"
require "yaml"

YAML_EXAMPLE = <<-YAML
a: 4
b: Hello
c: [1, 2, 3]
d:
  - Item one
  - Item two
e:
f: 3.5
g:
  # And a comment
h: A final value
YAML

pp YAML.load(YAML_EXAMPLE)

And this resulted in:

{"a"=>4,
 "b"=>"Hello",
 "c"=>[1, 2, 3],
 "d"=>["Item one", "Item two"],
 "e"=>nil,
 "f"=>3.5,
 "g"=>nil,
 "h"=>"A final value"}

Clearly, empty properties get value Nil. Which sort of makes sense. You cannot discern what type the property should have using the YAML data only, so the only possible value would be some sort of NULL value.

Do you think empty properties are likely a user mistake? Or is it nice that a user can have these empty properties in their templates so they know they're there and can get a value in the future?

Either way, what should we do?

  1. Don't allow empty properties. This could mean that paru/pandocomatic deviates in behavior from pandoc itself in this respect
  2. Warn users whenever an empty property is encountered, but keep as is.
  3. Use a default value for empty properties. For the pandoc options, we can. They are listed in paru's pandoc_options_version_2.yaml file. For other properties, we can keep the nil value.
  4. Combine 2 and 3.
iandol commented 2 years ago

In the case of this user, he was enabling and disabling filters using comments which seems like a useful way to make the most of a list (don't delete an entry but toggle it with a comment), so not a mistake. I often comment lines but keep them for context etc... I doubt users would do this accidentally, so I suspect that 3. on your list would be fine. I don't think 2. is really necessary personally, but also wouldn't complain if you prefer to emit a warning (ideally in verbose mode only). Thanks as always!!!

htdebeer commented 2 years ago

I think I fixed this issue in both pandocomatic and paru. Because the behavior of both packages is changed, I upgraded the minor version. However, I don't expect any behavioral change with existing code and setups.

iandol commented 2 years ago

Great, thanks Huub!