Fixes #359. Explicit unregistration of extensions. #564

robertpanzer · 2017-07-19T16:54:11Z

A second approach to unregistering extensions.
Now it is completely explicit.
Instead of getting a registration handle from the registration, a registration name has to be passed to the registration methods explicitly.

Basically all registration methods are overloaded with an additional version that takes a string as the registration name.

Note that this PR is for master.

mojavelinux · 2017-07-20T19:05:14Z

Great work!

I saw the problem you reported in core regarding the type coercion of the group name, which I'll address in the 1.5.6.1 release.

My one suggestion is to use the term "group name" instead of "registration name". That would mean changing the unregisterExtension method to:

void unregisterExtensionGroup(String name);

Although right now each group has one extension (when using this API), in the future we may merge subsequent registrations against the same group so the group holds multiple extensions.

That reads a bit better to me. Aside from that, I think this looks great.

mojavelinux · 2017-07-20T19:07:17Z

There's one possible alternative to consider (or add). We may want to introduce the concept of an extension group, to which you'd assign a name, then register one or more extensions. This could be an alternative to adding the group name to each registration method. Instead, you'd only get to use a group name if you create a group, register extensions on that group, then register the group. That's really what is happening under the covers anyway.

In other words, group registration would be a separate chain in the API, started by initiating a group.

mojavelinux · 2017-07-20T19:17:21Z

Actually, I really like this idea of starting with the group API. We'd use a createGroup method to create a group. That method would accept an optional group name argument. If a name is not specified, one is generated. Either way, the group name is accessible via the ExtensionGroup#getName. When you're done adding extensions to the group, you'd pass it to the register method (or perhaps registration is implicit by creating the group).

We could then add the "a la carte" groups like you've done in this pull request later (or, if you really want them, we can leave them too).

wdyt?

robertpanzer · 2017-07-20T21:43:05Z

Sounds good.
With a Java 6 codebase I'd prefer to register eagerly ;), but lazy registration should still be feasible.
I'll try to go for it as soon as i find the time, shouldn't be too much.
Your idea is sth like this, right?

ExtensionGroup g = asciidoctor.createGroup().block("yell").preprocessor(...);
g.register();
asciidoctor.convert(...);
g.unregister();

(On a small screen, please excuse any typos)

robertpanzer · 2017-07-20T21:44:31Z

^^ The call to .block() certainly also has to take a class or instance.

mojavelinux · 2017-07-20T22:26:23Z

Precisely.

I'm okay with registering eagerly. That's up to you. But the typed ExtensionGroup for register/unregister certainly has a nice feel to it.

robertpanzer · 2017-07-21T07:23:12Z

@mojavelinux I updated the PR with the ExtensionGroup.
The extensions are now registered lazily.

It certainly needs more test cases.

mojavelinux · 2017-07-27T18:29:19Z

asciidoctorj-core/src/main/java/org/asciidoctor/Asciidoctor.java

+     * Unregisters the extension represented by the given registration name.
+     * @param registrationName
+     */
+    void unregisterExtension(String registrationName);


Should this be groupName?

Actually this method should go away completely.
Unregistration should only be possible for extension registered via the ExtensionGroup mechanism.
Otherwise I would also need to add the "named" registration methods to JavaExtensionRegistry and RubyExtensionRegistry and I wanted to avoid that and keep everything in the ExtensionGroup.

mojavelinux · 2017-07-27T18:30:26Z

asciidoctorj-core/src/main/resources/org/asciidoctor/internal/asciidoctorclass.rb

@@ -4,7 +4,9 @@ module Extensions
        include_package 'org.asciidoctor.extension'
        # Treeprocessor was renamed in to TreeProcessor in https://github.com/asciidoctor/asciidoctor/commit/f1dd816ade9db457b899581841e4cf7b788aa26d
        # This is necessary to run against both Asciidoctor 1.5.5 and 1.5.6
-        TreeProcessor = Treeprocessor
+        if !defined? TreeProcessor


This is better written as:

TreeProcessor = Treeprocessor unless defined? TreeProcessor

mojavelinux · 2017-07-27T18:32:08Z

asciidoctorj-core/src/main/resources/org/asciidoctor/internal/asciidoctorclass.rb

+    end
+
+    def docinfo_processor(extensionName, groupName = nil)
+        Asciidoctor::Extensions.register (groupName != nil ? groupName.to_sym : nil) do


Don't put a space before the opening bracket. That causes a warning in some versions of Ruby.

You no longer need to coerce to a symbol as Asciidoctor 1.5.6.1 does that for you. Just pass the groupName as is.

Good to know, thanks!
I put the space in there not for the method argument, but to wrap the conditional expression.
I will try to get around without it.

In that case, you can write it as:

Asciidoctor::Extensions.register(groupName && groupName.to_sym)

or

Asciidoctor::Extensions.register(groupName ? groupName.to_sym : nil)

A method argument list is an implicit grouping expression.

Completely overseen that I can remove the coercion. Will remove it, then I don't need the brackets anyway.

It does require 1.5.6.1 though. To be clear, the coercion was fixed internally in 1.5.6.1.

mojavelinux · 2017-07-27T18:32:45Z

build.gradle

@@ -45,7 +45,7 @@ ext {
  xmlMatchersVersion = '1.0-RC1'

  // gem versions
-  asciidoctorGemVersion = project.hasProperty('asciidoctorGemVersion') ? project.asciidoctorGemVersion : '1.5.5'
+  asciidoctorGemVersion = project.hasProperty('asciidoctorGemVersion') ? project.asciidoctorGemVersion : '1.5.6'


Let's use 1.5.6.1.

Yes, that's part of the other PR, that's why I sticked with 1.5.6 here.

mojavelinux · 2017-07-27T18:33:02Z

This is looking really nice!

I made some comments inline.

robertpanzer · 2017-07-28T07:14:28Z

Updated the PR implementing the review comments.

robertpanzer · 2017-07-28T08:21:48Z

Bumped to Asciidoctor 1.5.6.1 and removed the coercion.

robertpanzer · 2017-07-28T08:55:40Z

I think it would make sense to squash this PR and put it on top of #563
Otherwise I'll never get a green build without pulling the changes from that PR in here.

mojavelinux · 2017-07-29T08:29:43Z

Seems reasonable to me!

robertpanzer · 2017-07-29T14:39:18Z

Integrated into #563
Therefore closing this PR.

Fixes asciidoctor#359. Explicit unregistration of extensions.

ccf7bfa

Introduce ExtensionGroup

9942d0a

mojavelinux reviewed Jul 27, 2017

View reviewed changes

Implemented review comments

f495298

Remove coercion of extension group names to symbols

13eada6

robertpanzer closed this Jul 29, 2017

robertpanzer mentioned this pull request Jul 29, 2017

Upgrade to Asciidoctor 1.5.6 and Asciidoctor-Diagram 1.5.5 #563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #359. Explicit unregistration of extensions. #564

Fixes #359. Explicit unregistration of extensions. #564

robertpanzer commented Jul 19, 2017

mojavelinux commented Jul 20, 2017

mojavelinux commented Jul 20, 2017

mojavelinux commented Jul 20, 2017 •

edited

Loading

robertpanzer commented Jul 20, 2017

robertpanzer commented Jul 20, 2017

mojavelinux commented Jul 20, 2017

robertpanzer commented Jul 21, 2017

mojavelinux Jul 27, 2017

robertpanzer Jul 28, 2017

mojavelinux Jul 28, 2017

mojavelinux Jul 27, 2017

mojavelinux Jul 27, 2017

robertpanzer Jul 28, 2017

mojavelinux Jul 28, 2017

robertpanzer Jul 28, 2017

mojavelinux Jul 28, 2017

mojavelinux Jul 28, 2017

mojavelinux Jul 27, 2017

robertpanzer Jul 28, 2017

mojavelinux Jul 28, 2017

mojavelinux commented Jul 27, 2017

robertpanzer commented Jul 28, 2017

robertpanzer commented Jul 28, 2017

robertpanzer commented Jul 28, 2017

mojavelinux commented Jul 29, 2017

robertpanzer commented Jul 29, 2017

Fixes #359. Explicit unregistration of extensions. #564

Fixes #359. Explicit unregistration of extensions. #564

Conversation

robertpanzer commented Jul 19, 2017

mojavelinux commented Jul 20, 2017

mojavelinux commented Jul 20, 2017

mojavelinux commented Jul 20, 2017 • edited Loading

robertpanzer commented Jul 20, 2017

robertpanzer commented Jul 20, 2017

mojavelinux commented Jul 20, 2017

robertpanzer commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojavelinux commented Jul 27, 2017

robertpanzer commented Jul 28, 2017

robertpanzer commented Jul 28, 2017

robertpanzer commented Jul 28, 2017

mojavelinux commented Jul 29, 2017

robertpanzer commented Jul 29, 2017

mojavelinux commented Jul 20, 2017 •

edited

Loading