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

Implement Cloneable in org.asciidoctor.Options #1279

Open
Marfien opened this issue Jul 31, 2024 · 6 comments
Open

Implement Cloneable in org.asciidoctor.Options #1279

Marfien opened this issue Jul 31, 2024 · 6 comments

Comments

@Marfien
Copy link

Marfien commented Jul 31, 2024

Hello there.

I recently encountered multiple scenatios where I needed an org.asciidoctor.Options instance to be (almost) the same as the original. Unfortunately, the only way of doing this is to use the internal/deprecated methods involving the underlying map.
Is there any intended way of doing this? Otherwise, I'd be happy to open a PR ;)

Kind Regards

@robertpanzer
Copy link
Member

I don't think it is a good idea to clone an Options object.
AsciidoctorJ will mutate it before passing it to Asciidoctor Ruby, and I cannot tell what Asciidoctor Ruby will do with this object.
I think it's best if the caller of the API builds an abstraction if they need slightly different options between 2 conversions.

@Marfien
Copy link
Author

Marfien commented Aug 1, 2024

Hey @robertpanzer,

What would you think about removing the underlying Options-Instance of org.asciidoctor.OptionsBuilder and replacing it with a map that will be passed to a new created instance everytime the OptionsBuilder#build() method is invoked?

@robertpanzer
Copy link
Member

I am still reluctant to do that.
The same problem applies to the attributes property that is a member of Options.

@cstettler
Copy link

I also thought the same way as @Marfien: collect the state passed to OptionsBuilder and only create the Options instance and the attributes when invoking build(). If the same would be done for AttributesBuilder, the issue regarding the attributes map would also be solved (or instead or in addition copying the map before setting it as to the OptionsBuilder). That way, users could pass on the OptionsBuilder instance, modified it where needed and always create a fresh and decoupled instance of Options when calling AsciidoctorJ APIs.

Being forced to create your own abstraction is very inconvenient and somehow defeats the purpose of having a convenient Java API for the underlying Ruby AsciiDoc API, don't you think?

@abelsromero
Copy link
Member

I cannot tell what Asciidoctor Ruby will do with this object

I think this needs clarification. Even if Options is guaranteed to be unique, we cannot guarantee it won't be mutated by the core. In the example below, you may expect op to be the same in both calls, but that's not true.

Options op = optionsBuilder.build()

convert(op);
convert(op);

Being forced to create your own abstraction is very inconvenient and somehow defeats the purpose of having a convenient Java API for the underlying Ruby AsciiDoc API, don't you think?

I agree, but since the underlying issue of mutability remains, I don't think any change in Builder will suffice. If anything I'd take the radical approach to remove Builders 🪓

@Marfien
Copy link
Author

Marfien commented Aug 19, 2024

What do you think about the approach of making a copy of the Options instance internally every time the underlying Ruby API?
Or maybe the current Options implementation could be modified to be used as a Wrapper for an internally used Ruby-Options?

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

No branches or pull requests

4 participants