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

resolves #844 map Document.playbackAttributes #853

Merged
merged 2 commits into from
Mar 17, 2020
Merged

resolves #844 map Document.playbackAttributes #853

merged 2 commits into from
Mar 17, 2020

Conversation

ggrossetie
Copy link
Member

resolves #844

packages/core/src/asciidoctor-core-api.js Show resolved Hide resolved
packages/core/src/asciidoctor-core-api.js Outdated Show resolved Hide resolved
packages/core/src/asciidoctor-core-api.js Outdated Show resolved Hide resolved
packages/core/src/asciidoctor-core-api.js Outdated Show resolved Hide resolved
@mojavelinux
Copy link
Member

I'll admit I'm a bit uneasy about mapping these APIs. The APIs and the behavior were never meant to be public. (That's why they seem so awkward). Perhaps we can add a disclaimer to them that it's an advanced feature, then focus our attention the proper solution upstream, which is to proxy the document attributes so they reflect the correct state when viewed from the tree.

@djencks
Copy link
Contributor

djencks commented Mar 10, 2020

I'm uneasy too. Rather than (only) marking them 'advanced' I think it would be better to indicate that a better solution is hoped for in the future and these methods may disappear.

@ggrossetie
Copy link
Member Author

I'll admit I'm a bit uneasy about mapping these APIs. The APIs and the behavior were never meant to be public. (That's why they seem so awkward).

I agree but right now they can be useful in some scenarios

Perhaps we can add a disclaimer to them that it's an advanced feature, then focus our attention the proper solution upstream, which is to proxy the document attributes so they reflect the correct state when viewed from the tree.
I'm uneasy too. Rather than (only) marking them 'advanced' I think it would be better to indicate that a better solution is hoped for in the future and these methods may disappear.

Sure, as soon as Asciidoctor core provides a better alternative, I will remove these methods and map the new ones.
I guess that Asciidoctor core will remove these methods in a major release (because even if they are not widely used, they are marked as "public").

Do you have suggestions about what we should add in the JavaScript documentation?

/**
 * Replay attribute assignments at the block level.
 * /!\ This low-level API will probably be removed in the next major version /!\
 */

@mojavelinux
Copy link
Member

mojavelinux commented Mar 12, 2020

I agree but right now they can be useful in some scenarios

I definitely recognize the need.

I guess that Asciidoctor core will remove these methods in a major release (because even if they are not widely used, they are marked as "public").

We really have no idea at this point.

They are public only because they haven't been updated to be private. Remember that prior to Asciidoctor 2, there was no delineation. So it's more that a decision just hasn't been made yet.

Do you have suggestions about what we should add in the JavaScript documentation?

I think it should be a very direct message.

This method belongs to an internal API that deals with how attributes are managed by the processor. If you understand why this group of methods are necessary, and what they do, feel free to use them. However, keep in mind they are subject to change at any time.

@ggrossetie
Copy link
Member Author

@mojavelinux Sounds good, thanks for your input! I will update the documentation to use a clear and direct message as suggested.

@ggrossetie ggrossetie merged commit c2c788f into asciidoctor:master Mar 17, 2020
@ggrossetie ggrossetie deleted the issue-844-playback-attrs branch March 17, 2020 14:32
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.

Map Document#playback_attributes
3 participants