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

Add feature to convert from condition to ifdef for #43 #54

Conversation

junaruga
Copy link
Member

@junaruga junaruga commented Jun 2, 2016

I added new feature about condition.
#43

The "condition" of DocBook XSL is converted to "ifdef".

There are several restriction for usage.

  • The condition is not supported for several XML nodes, and cases. For example.
    • The node itself is not supported for DocBookRX itself that warns "No visitor defined for <#{node.name}>! Skipping.", it does not work.
    • subtitle node in section, productname,
    • xi:include (include tag looks supported, but xi:include looks not supported.)

Could you check my code?
Thanks.

@junaruga junaruga force-pushed the feature/convert-from-condition-to-ifdef-upstream branch from 4358efa to 81c0ade Compare June 2, 2016 15:27
@mojavelinux
Copy link
Member

Overall, this looks good. I think we should use the single-line conditional in the case of inline elements like phrase.

a
ifdef::condition[b]
c

Does that seem reasonable?

@junaruga
Copy link
Member Author

junaruga commented Jun 3, 2016

Hi, @mojavelinux Thanks for your checking.

Well, when I compared single line: ifdef , and multi line ifdef,

Comparing single line ifdef and multi line ifdef

single line ifdef

:foo:

a
ifdef::foo[b]
c

multi line ifdef

:foo:

a
ifdef::foo[]
b
endif::foo[]
c

both cases will get below same result. I checked it by asciidoctor firefox plugin.

a b c

However, multi line ifdef looks better than single line ifdef.
Because

  1. Multi ifdef can support to display multi lines in the context of ifdef.
  2. behavior of "]" in the ifdef context, is easier to understand.

Merit 1

For example of merit 1, in below case, user can expect to display the context (2 lines) with condition "foo".

<para condition="foo">
Today is Friday.
Tomorrow is Saturday.
</para>

But below actual generated result does not work.

:foo:

ifdef::foo[Today is Friday.
Tomorrow is Saturday.]

This actual result for html with asciidoctor firefox plugin, is ifdef::foo[Today is Friday. Tomorrow is Saturday.]. because single-line ifdef is not recognized.
It seems that single-line ifdef exists for single line context.

I do not want to use below way that works in this case, because this looks more complicated.

ifdef::foo[Today is Friday.]
ifdef::foo[Tomorrow is Saturday.]

However below multi line ifdef works for this case, easily.

:foo:

ifdef::foo[]
Today is Friday.
Tomorrow is Saturday.
endif::foo[]

Merit 2

For example of merit 2 you can see in below case.

:foo:

a
ifdef::foo[b]c]
d

This actual result works.
"a b]c d" is displayed.
But
When open the adoc file with vim, vim syntax highlight for adoc mode, fails to show correct highlight.
So, this is the evidence that behavior of "]" in the single line ifdef context, is not clearly for users including vim itself.

So, I prefered "multi line ifdef" than single line ifdef in this time.

How do you think?

@junaruga
Copy link
Member Author

junaruga commented Jun 6, 2016

Hi, @mojavelinux
How do you think?
If you agree my opinion, and you like to use multi line ifdef, as I found conflicts between my pull-request and current master, I can solve the conflict, and can do rebase my branch for the pull-request, by myself.

@junaruga
Copy link
Member Author

Hi, @mojavelinux ( Cc: @mrietveld )
After my previous comment, 7 days has been passed.
I would like you to merge this modification to master branch.
Is there any problem for you?

Thanks.

@mrietveld
Copy link
Contributor

Merged @ 01412b9

@mrietveld mrietveld closed this Jun 28, 2016
@junaruga
Copy link
Member Author

@mrietveld
Thanks!!

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