Skip to content

#158 implement yaml page variable merging #261

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

djencks
Copy link
Contributor

@djencks djencks commented Feb 13, 2021

using asciidoctor.merge_attributes key (#158 partial)

@djencks djencks requested a review from mojavelinux February 13, 2021 23:44
@djencks djencks force-pushed the issue-158-nested-attributes branch from 3446187 to a5c0576 Compare February 14, 2021 01:20
@mojavelinux
Copy link
Member

I think merge_attributes should only be a list of attribute names. The definition of the attribute itself should still be under the attributes key. Otherwise, the "merge_attributes" is doing double duty and that just feels awkward. In other words, it should look like:

asciidoc:
  attributes:
    page-header:
      foo: bar
      yin: yang
  merge_attributes: [page-header]

@mojavelinux
Copy link
Member

...and maybe even mergeable_attributes

@djencks
Copy link
Contributor Author

djencks commented Feb 16, 2021

I think merged_attributes might be the best option so far. mergeable_attributes to me leaves in doubt whether they will in fact be merged. I'll update the name once we decide what approach to take with the harder stuff.

I've provided 2 more PRs that show 2 ways of implementing your suggestion of having only the attribute name under merge_attributes. One does not provide the _config.yml value to the page, just like this PR, which feels odd for an attribute under attributes. The other provides the stringified value to the page, which feels extremely odd in comparison with the Object values for other attributes attributes. However, in this case, I don't see any other way to tell Asciidoctor that the value can be overriden, as appending '@' to an object doesn't make sense. This implementation is also significantly more complex than the other two, which are roughly comparable. Ideally there would be a way to supply an Object attribute value to Asciidoctor and say that it can be overridden. I believe extensions can get the actual attribute value as an object.

#264

#265

Another question is where the merge[d]_attributes key should go. I put it under asciidoctor only. I think it's a bit bizarre to have attributes work under both asciidoc and asciidoctor, so I'd rather choose only one for the new key, and since I don't see this behavior necessarily working for other converter implementations I'd prefer asciidoctor.

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.

2 participants