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

Fixed issue #15 Upgrade to Asciidoctor 2.2.0 #17

Merged
merged 13 commits into from
May 13, 2020

Conversation

henriette-einstein
Copy link
Contributor

No description provided.

@henriette-einstein henriette-einstein linked an issue May 10, 2020 that may be closed by this pull request
package.json Outdated Show resolved Hide resolved
doc/documentation.adoc Outdated Show resolved Hide resolved
doc/documentation.adoc Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@henriette-einstein
Copy link
Contributor Author

henriette-einstein commented May 10, 2020

@Mogztter I get an error if I try to run the test case with a converter that is exported as a class. I don't know, why that is the case. Do you have any idea?

It says:
convert: undefined method `convert' for #Proc:0x6b8
even though I explicitly checked for the existence of the function before.

When I export the class via a function, it works.

@ggrossetie
Copy link
Member

@henriette-einstein
Copy link
Contributor Author

There is a test case included in this branch, that fails when you run "yarn test". It is the same as in the previous release (the master branch). It worked with Asciidoctor 2.1.2.

To reproduce, you can just checkout this branch and run "yarn test" or "npm test".

I can isolate that test and add some more stuff if that will help. Just give me notice.

@henriette-einstein
Copy link
Contributor Author

I just saw, that you are passing in a class.
In my testcase, I create a new instance using new(). I'll check if that behaves the same as in your case.

@henriette-einstein
Copy link
Contributor Author

henriette-einstein commented May 10, 2020

I tweaked the code a little for debugging. Now I get the following "stacktrace"

 convert: undefined method `convert' for #<Proc:0x6b6>
  at Function.$method_missing (node_modules/asciidoctor-opal-runtime/src/opal.js:3907:56)
  at Function.method_missing_stub (node_modules/asciidoctor-opal-runtime/src/opal.js:1310:35)
  at constructor.$convert (node_modules/@asciidoctor/core/dist/node/asciidoctor.js:8418:35)
  at Function.$convert (node_modules/@asciidoctor/core/dist/node/asciidoctor.js:16304:22)
  at Function.Asciidoctor.convert (node_modules/@asciidoctor/core/dist/node/asciidoctor.js:21693:21)
  at Transform._transform (index.js:86:28)
  at Transform._read (_stream_transform.js:191:10)
  at Transform._write (_stream_transform.js:179:12)
  at doWrite (_stream_writable.js:385:12)
  at writeOrBuffer (_stream_writable.js:367:5)
  at Transform.Writable.write (_stream_writable.js:307:12)
  at Context.<anonymous> (spec/gulp-asciidoctor.spec.js:393:12)
  at processImmediate (internal/timers.js:456:21)

Another info:
I added another testcase (it('should register a custom converter instantiated here',) in which I instantiate the converter in the test case itself (line 406). That works. If I pass the class and instantiate it in index.js, the test fails. (This can be reproduced if you change line 50 in index.js to

asciidoctor.ConverterFactory.register(new options.converter(), [asciidoctorOptions.backend])

and line 406 of gulp-asciidoctor-spec.js to

  converter: CnvClass

@ggrossetie
Copy link
Member

@henriette-einstein Thanks I will checkout your branch tomorrow to try to understand why it's failing.

@henriette-einstein
Copy link
Contributor Author

henriette-einstein commented May 11, 2020

I found it! That is probably a pathological edge-case.

What happened is basically

const Clazz = MyClass
const myInstance = new Clazz()
asciidoctor.ConverterFactory.register(myInstance, [asciidoctorOptions.backend])
asciidoctorOptions = {
   converter: Clazz
}
asciidoctor.convert(content, asciidoctorOptions)

In that case, I have a class that is instantiated and registered before the class itself is passed as an option to the asciidoctor processor.

I don't know if such a case could be checked in your code.

I removed the option "converter" after the registration. Then it works.

Sorry for the disturbances, it was my fault.

@henriette-einstein
Copy link
Contributor Author

I did not know, that "converter" was actually an option that is used by the underlying Asciidoctor. I therefore changed the name of the plugin option to "cnv" to avoid problems.

@ggrossetie
Copy link
Member

I did not know, that "converter" was actually an option that is used by the underlying Asciidoctor. I therefore changed the name of the plugin option to "cnv" to avoid problems.

Yes converter is an option but if you do that you will need to pass a "Ruby" class or object. The asciidoctor.ConverterFactory.register function will "lift" the JavaScript class or instance to an Opal class/object.

----

== Usage
This plugin can be used to integrate Asciidoctor processing in yout gulp tasks. Here is a typical usage scenario.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This plugin can be used to integrate Asciidoctor processing in yout gulp tasks. Here is a typical usage scenario.
This plugin can be used to integrate Asciidoctor processing in your gulp tasks. Here is a typical usage scenario:

exports.process = gulp.parallel(processAdocFiles, copyImages)
----

You can configure the plugin itself by just adding options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You can configure the plugin itself by just adding options
You can configure the plugin itself by adding options:

...
----

And you can also configure the underlying Asciidoctor processor via the respective Asciidoctor options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
And you can also configure the underlying Asciidoctor processor via the respective Asciidoctor options
And you can also configure the underlying Asciidoctor processor via the respective Asciidoctor options:

The following options can be used to configure the Gulp-Plugin

extension:: Sets the output extension of the plugin. Defaults to '.html'
cnv:: Sets a custom converter that should be used for processing. Defaults to undefined.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cnv:: Sets a custom converter that should be used for processing. Defaults to undefined.
cnv:: Sets a custom converter that should be used for processing. Defaults to `undefined`.

cnv:: Sets a custom converter that should be used for processing. Defaults to undefined.

== AsciiDoctor Options
All options of the underlying AsciiDoctor can be set. Please refer to the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
All options of the underlying AsciiDoctor can be set. Please refer to the
All options of the underlying Asciidoctor can be set. Please refer to the

All options of the underlying AsciiDoctor can be set. Please refer to the
https://asciidoctor-docs.netlify.app/asciidoctor.js/processor/convert-options[Converter options] page to see, which options are available.

However, the Asciidoctor options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
However, the Asciidoctor options
However, the following Asciidoctor options:

== Important

=== Base Directory
Do not forget to set the AsciiDoctor option `base_dir` if you want to include
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Do not forget to set the AsciiDoctor option `base_dir` if you want to include
Do not forget to set the Asciidoctor option `base_dir` if you want to include

* If 'header_footer' is set and 'standalone' is not set, the processor will receive 'standalone' = value of 'header_footer' option and the option 'header_footer' will be stripped.
* If both 'header_footer' and 'standalone' are set, the option 'header_footer' will be stripped.

== Change Log
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's one word (not 100% sure):

Suggested change
== Change Log
== Changelog

@ggrossetie
Copy link
Member

@henriette-einstein The code looks fine, I only have minors nitpicks about the documentation, otherwise 👍

@henriette-einstein
Copy link
Contributor Author

@Mogztter I am ready to merge the pull request if that is ok for you

@ggrossetie
Copy link
Member

@henriette-einstein It looks good to me, I think you can merge it! Well done 👍

@henriette-einstein henriette-einstein merged commit c5015d7 into master May 13, 2020
This was linked to issues May 13, 2020
@ggrossetie ggrossetie deleted the asciidoctor_2_2_0-branch branch May 13, 2020 11:13
@ggrossetie
Copy link
Member

🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants