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

null / "" are not mapped to false / true correctly? #125

Closed
ascheman opened this issue Mar 26, 2017 · 9 comments
Closed

null / "" are not mapped to false / true correctly? #125

ascheman opened this issue Mar 26, 2017 · 9 comments

Comments

@ascheman
Copy link

Having an issue (asciidoctor/asciidoctor-maven-plugin#288) with building slides from asciidoc with the Maven plugin, @mojavelinux pointed out that the plugin mapping of boolean values is correct but values are not mapped correctly by the reveal converter:

E.g., setting

<revealjs_controls>true</revealjs_controls>

leads to

Reveal.initialize({
  // Display controls in the bottom right corner
  controls: ,

making JS complain about the , instead of an expected value of true or false.

This results in an empty slide show.

@mojavelinux
Copy link
Member

The correct way to emit these values is as follows:

#{(attr 'revealjs_controls', '') ? true : false}

or

#{!!(attr 'revealjs_controls', '')}

(!! coerces the value to a boolean)

That way, the expression emits "true" if the attribute has a value (even if that value is empty string) and "false" if the value is nil or the attribute is not set.

@mojavelinux
Copy link
Member

(we don't have to worry about converting the boolean to a string since the interpolation already takes care of that for us).

@mojavelinux
Copy link
Member

This also shields us from being hit by other bogus values for these attributes.

@mojavelinux
Copy link
Member

Actually, because we use default values, this isn't going to work since there's no way in what I proposed to set a false value.

That means we need the following:

#{(attr 'revealjs_controls', '') != 'false'}

@mojavelinux
Copy link
Member

Default values for attributes are still a bit rocky in Asciidoctor, which is what is making this so difficult. There's also the legacy that true values are empty string. We're just jammed between a rock and a hard place on this one...so we have to do some extra work.

@obilodeau
Copy link
Member

I don't understand what causes the issue. Can someone provide a simple example without the maven part?

@mojavelinux
Copy link
Member

In AsciiDoc, empty string represents a "true" value. However, we are passing this value through "as is", thus causing the Reveal.js configuration to look like:

Reveal.initialize({
  // Display controls in the bottom right corner
  controls: ,

Instead, we should coerce the value to a boolean using a helper function. I've proposed a PR. See #126.

Here's how we now map the values:

true => true
false => false
nil => false
'true' => true
'false' => false
1 => true
0 => false
'1' => true
'0' => false

(technically, any other value is true).

obilodeau added a commit that referenced this issue Mar 30, 2017
resolves #125 coerce Reveal.js settings to boolean values
@obilodeau
Copy link
Member

Fixed pushed in master. Let me know if a release would simplify your workflow. I can make one.

@ascheman
Copy link
Author

ascheman commented Apr 5, 2017

Thanks so far, can wait for the next release ...

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

No branches or pull requests

3 participants