inukshuk / citeproc-ruby

A Citation Style Language (CSL) Cite Processor
103 stars 23 forks source link

Don't treat <choose> as a grouping/rendering element #83

Closed bwiernik closed 2 months ago

bwiernik commented 2 months ago

Noticed this behavior here: https://github.com/citation-style-language/styles/pull/6500

For this structure:

<macro name="publisher">
    <group delimiter=", ">
      <choose>
        <if type="article-journal article-magazine" match="none">
          <text variable="genre"/>
          <text variable="publisher"/>
          <text variable="publisher-place"/>
        </if>
      </choose>
    </group>
  </macro>

The expected output is: Genre, Publisher, Place

The group attributes like delimiter should pass through <choose> and apply to the individual rendering elements inside, with the same behavior as if the elements were supplied directly without <choose>. <choose> should not create an implicit <group>.

I find that the current behavior not infrequently produces surprising output for the checks we run on the CSL style repository. Would it be possible to update citeproc-ruby's behavior here?

Thanks!

inukshuk commented 2 months ago

Thanks @bwiernik -- it's been awhile since I've looked at this so my apologies while I get up to speed! Is the issue here that the three text nodes are rendered without the delimiter? Specifically, this note in the spec isn't being followed when rendering the text nodes?

bwiernik commented 2 months ago

Yes the issue is that the 3 nodes are being rendered without the delimiter

inukshuk commented 2 months ago

Yes you're right, I added your example as a test case and it's definitely rendering without the delimiter at the moment.

inukshuk commented 2 months ago

OK your example should be easy to fix, however, more generally, are ancestor delimiters computed based on the style structure only or based on the current rendering tree? For example if I omit the <group> in your example so that the macro contains only the <choose> element and if I then render that macro inside a group, then would you expect to find the delimiter attribute on that node?

inukshuk commented 2 months ago

I guess, using the rendering tree (i.e., inherit delimiters from outside marcros) is potentially dangerous, because this way you'd always reach at least the <layout> and therefore layouts with delimiters couldn't easily have choose blocks without a delimiter (they'd basically have to add extra <group> elements without delimiters). So I guess the intention is for delimiters to be inherited only through the style nodes?

inukshuk commented 2 months ago

OK I implemented it this way for now. Delimiters can't be inherited from outside a macro, so basically, the when a choose node produces output from more than one rendering element, they are joined using the delimiter of the closest group or layout element above it. If that's the way it's supposed to be we can close this and I'll push a new version of the Gem.

bwiernik commented 2 months ago

That's correct, there is no need to propagate delimiters into a macro call.

bwiernik commented 2 months ago

Thanks for looking into this so quickly!

inukshuk commented 2 months ago

Great, thanks!

I pushed version 2.1.0 to RubyGems.