Skip to content

Issue 158 nested attributes 3 #265

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 3 commits into
base: main
Choose a base branch
from

Conversation

djencks
Copy link
Contributor

@djencks djencks commented Feb 16, 2021

This implementation has only the attribute names in merge_attributes and supplies a stringified version of the _config.yml attribute value to the pages. This is the only way I see to tell Asciidoctor that the value can be overridden. This behavior is inconsistent with other object valued attributes, which can be accessed as objects by extensions. However, if the value is overridden for merging by a page header attribute, the value would be a string. Perhaps stringifying the object into yaml would be more useful than using Ruby's .to_s operator.

@mojavelinux
Copy link
Member

This is the only way I see to tell Asciidoctor that the value can be overridden.

Asciidoctor now allows the override modifier to be set on the key instead of the value (for this very reason). It may still be necessary to mix in a module the value to respond to any methods that the core processor tries to call on it. But I believe it is possible to preserve the value and communicate that it can be overridden.

@djencks
Copy link
Contributor Author

djencks commented Feb 22, 2021

I can't speak to the motivation behind the code that allows '@' on keys as well as values, but the effect is to convert the value to a string and append '@':

            key, val = key.chop, %(#{val}@)

(line 303, document.rb)
Trying to tie into this would result in the same effect as the current PR, but with longer and harder to understand code.
I haven't been able to find documentation on what classes the %() and #{} operators might be attached to, but it doesn't really seem advisable to me to try to circumvent with obscure ruby magic this evident intent that attributes be strings .

IMO if there was a real intent to allow object attribute values, the '@' handling in Asciidoctor would have been rewritten so that the overridability of an attribute_overrides entry would be in a flag rather than encoded in the attribute value. I actually think that both this and coercing all supplied attributes to string upfront make the most sense.

I don't think the current behavior of allowing objects as attribute values and then converting them to strings if they happen to be overridable makes any sense. I think that either:

  • object valued attributes should not be supplied to Asciidoctor at all, whether or not they are overridable
    or
  • objects should be stringified into single-line yaml.

Unfortunately these are both breaking changes.

@mojavelinux
Copy link
Member

IMO if there was a real intent to allow object attribute values, the '@' handling in Asciidoctor would have been rewritten so that the overridability of an attribute_overrides entry would be in a flag rather than encoded in the attribute value. I actually think that both this and coercing all supplied attributes to string upfront make the most sense.

I agree that we should not be trying to convince core to allow object values as values are, by definition, strings.

I realize this is a tough situation. I just wanted to emphasize that we did at the @ modifier to the key since it's a clearer way to express the intent. Having it on the value is more awkward, IMO.

@djencks
Copy link
Contributor Author

djencks commented Feb 23, 2021

I certainly agree that key@ is clearer than value@!
Thanks for confirming that Asciidoc attributes are intended to be strings.
My latest idea is to enable this merging feature behind a flag such as "unstable" or "experimental" and when set convert all object valued attributes to single-line yaml strings. When we get to version 4 we can make the behavior default unless we've come up with a better idea.

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