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

Simplify Options and Attributes creation and usage #977

Open
abelsromero opened this issue Nov 28, 2020 · 4 comments
Open

Simplify Options and Attributes creation and usage #977

abelsromero opened this issue Nov 28, 2020 · 4 comments

Comments

@abelsromero
Copy link
Member

abelsromero commented Nov 28, 2020

This is not a new thing, but is something I think we haven't given much thought. And while going through the docs I see the docs are convoluted because of how the code works. It's basically "You can use X,Y,Z in this ways...but it's all for the same.".

Asciidoctor.convert accepts Options, OptionsBuilder and Map<String,Object>. Same applies for convertDirectory, convertFile and convertFiles. This creates a lot of methods for the same and is not a bad thing on its own.
What I think requires re-working is that those method's usage is not obvious to new users, specially when using IDE autompletion. Furthermore, Options has a public normal constructor, but OptionsBuilder has a static initializer plus the get and asMap methods that return Options and Map.
Also, we have load and loadFile which only accept Map<String,Object>·

To make things easier and more Java-ish wtyt if we...

  • Make Ascidoctor methods only accept 1 type. I am thinking of Options for the sake of abstraction.
  • Make Options package protected to be used only internally by the builder.
  • Add public static builder method to Options. This will return an instance of OptionsBuillder.
    I am not sure if we should have a method that accepts map, one for free string-value for sure, accepting a map seems something out of convinience more than good design.
  • Rename get in OptionsBuilder to build following common Java conventions.
  • Make Options.map method package protected so it can only be used internally.
  • Overall remove use of maps from public components.

This should be marked for a 3.0 release since it may break integrations.

PS: same applies for Attributes, AttributesBuilder...

@robertpanzer
Copy link
Member

Totally agree!

@abelsromero
Copy link
Member Author

Given the scope, maybe we should mark this as branking change and 3.x.x release.

@robertpanzer
Copy link
Member

Yes, this is definitely a breaking change.
But we can already do a few things right now to guide users to the new route and make them aware that things might change in the future:

  • Deprecate the constructors of Options and Attributes.
  • Add methods OptionsBuilder.build() and AttributesBuilder.build()
  • Add methods OptionsBuilder.builder() and AttributesBuilder.builder().
  • Deprecate static methods OptionsBuilder.options() and AttributesBuilder.attributes().
  • Deprecate methods OptionsBuilder/AttributesBuilder.asMap() & .get().

Wdyt?

@abelsromero
Copy link
Member Author

  • Add methods OptionsBuilder.builder() and AttributesBuilder.builder().

For that what I had in mind is make *Builder constructors package protected. They were private, so this does not break compatibility.

Also added

  • Deprecate OptionsBuilder attributes(Map<String, Object> attributes) and OptionsBuilder attributes(AttributesBuilder attributes) . So only OptionsBuilder attributes(Attributes attributes) remains.

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

2 participants