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.

Sign up for GitHub

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

Jump to bottom

resolves #170, #180, #179, #168 & #160 enable configuration for site parser #182

Merged
merged 1 commit into from
Dec 15, 2015
Merged

resolves #170, #180, #179, #168 & #160 enable configuration for site parser #182

merged 1 commit into from
Dec 15, 2015

Conversation

mojavelinux
Copy link
Member

  • rewrite AsciidoctorParser (used by Maven Site Plugin) to make it more organized and extensible
  • specify baseDir by default when invoking Asciidoctor API
  • pass AsciiDoc configuration defined in Maven Site plugin to the Asciidoctor API
    • API options (e.g., templateDir)
    • Ruby options (e.g., requires)
    • AsciiDoc attributes (e.g., icons=font)
  • enhance Maven Site integration test
    • add custom template
    • add include
    • add source highlighting
    • make validation code compatible with Java 6
  • enable integration tests on Travis CI
  • remove unused legacy site module
  • fix license headers
  • minor cleanups

robertpanzer
robertpanzer reviewed Dec 14, 2015
View reviewed changes
src/main/java/org/asciidoctor/maven/site/AsciidoctorParser.java
if (!require.trim().isEmpty()) {
try {
// FIXME AsciidoctorJ should provide a public API for requiring paths in the Ruby runtime
RubyUtils.requireLibrary(JRubyRuntimeContext.get(), require.trim());
Copy link
Member

Choose a reason for hiding this comment

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

Since Asciidoctorj 1.5.3 there's a method Asciidoctor.requireLibrary(String)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change this to a TODO since we haven't upgraded the AsciidoctorJ version yet. As soon as we do, we'll switch it over.

@robertpanzer
Copy link
Member

👍 \o/

@mojavelinux
Copy link
Member Author

I agree, this is a major step forward. We just killed a whole army of bugs :)

@jmbruel
Copy link

jmbruel commented Dec 14, 2015

Congrats!👑

@mnhock
Copy link

mnhock commented Dec 14, 2015

👍

@mojavelinux
Copy link
Member Author

I think this is ready to go!

@abelsromero
Copy link
Member

I'm really glad you could find a solution for this. I agree this is a major step and something worth of a 1.5.3 release.
I have a couple of questions:

  • Should AsciidoctorMojo and AsciidoctorParser be refactored to use common methods?
  • Should we use RubyUtils.requireLibrary or Asciidoctor.requireLibrary (from AsciidoctorJ)?

@mojavelinux
Copy link
Member Author

Should we use RubyUtils.requireLibrary or Asciidoctor.requireLibrary (from AsciidoctorJ)?

We should use Asciidoctor.requireLibrary, but we have to do it after the fix for #183.

@mojavelinux
Copy link
Member Author

I added a note to #183.

@mojavelinux
Copy link
Member Author

Should AsciidoctorMojo and AsciidoctorParser be refactored to use common methods?

Eventually, yes. Though AsciidoctorParser has more specialized requirements given that it cannot benefit from the type conversion provided by the @parameter injection.

What I really like now about the AsciidoctorParser class is that it would be easy to extend given that all the steps are partitioned into methods. In other words, you have hooks into the logic in order to customize the behavior. The main plugin should be this way too.

@mojavelinux
resolves asciidoctor#170, asciidoctor#180, asciidoctor#179, asciidoct…
5e4a857 
…or#168 & asciidoctor#160 enable configuration for site parser

- rewrite AsciidoctorParser (used by Maven Site Plugin) to make it more organized and extensible
- specify baseDir by default when invoking Asciidoctor API
- pass AsciiDoc configuration defined in Maven Site plugin to the Asciidoctor API
  - API options (e.g., templateDir)
  - Ruby options (e.g., requires)
  - AsciiDoc attributes (e.g., icons=font)
- enhance Maven Site integration test
  - add custom template
  - add include
  - add source highlighting
  - make validation code compatible with Java 6
- enable integration tests on Travis CI and Appveyor
- remove unused legacy site module
- fix license headers
- minor cleanups
@mojavelinux
Copy link
Member Author

@abelsromero Let me know if you need more time to review. I'm ready to merge when you are.

@robertpanzer
Copy link
Member

Concerning Asciidoctor.requireLibrary or RubyUtils.requireLibrary(JRubyRuntimeContext.get()):

JRubyRuntimeContext.get() won't be available with AsciidoctorJ 1.6.0 as there is no longer one global Ruby runtime.
Instead there is a new method JRubyRuntimeContext.get(Asciidoctor) that will return the Ruby runtime associated to the given Asciidoctor instance.

So better to use the first approach Asciidoctor.requireLibrary(String).

@mojavelinux
Copy link
Member Author

👍

@abelsromero
Copy link
Member

@abelsromero Let me know if you need more time to review. I'm ready to merge when you are.

GO!

@mojavelinux
Copy link
Member Author

👍

@mojavelinux mojavelinux mentioned this pull request Dec 14, 2015
Merged
@LightGuard
Copy link
Member

Amazing the hoops you have to go through for this

mojavelinux added a commit that referenced this pull request Dec 15, 2015
@mojavelinux
Merge pull request #182 from mojavelinux/issue-170
8eb23d4 
resolves #170, #180, #179, #168 & #160 enable configuration for site parser
@mojavelinux mojavelinux merged commit 8eb23d4 into asciidoctor:master Dec 15, 2015
@mojavelinux mojavelinux deleted the issue-170 branch December 15, 2015 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mojavelinux @robertpanzer @jmbruel @mnhock @abelsromero @LightGuard