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

Adding sourcemap option when converting documents #264

Closed
AlexanderZobkov opened this issue Sep 11, 2016 · 18 comments
Closed

Adding sourcemap option when converting documents #264

AlexanderZobkov opened this issue Sep 11, 2016 · 18 comments
Milestone

Comments

@AlexanderZobkov
Copy link

I'm working on an extension (based on treeprocessor) that is supposed to be invoked via asciidoctor-maven-plugin. The extension should provide line number, asciidoctor file name where error is occurred.
If understand it right, sourcemap option should be specified to get line numbers from nodes. Looking on the maven plugin sources, it looks like there is no possibility to pass that option to asciidoctorj API.

While it's not possible to pass sourcemap option via the maven plugin configuration attributes, Could you please suggest if it is possible pass sourcemap option via Ruby code that could be injected by using "requires" attribute?

@abelsromero
Copy link
Member

I've been trying a couple of thing (e.g. Preprocessors), but without success. To my understanding requires initialises an Asciidoctor compatible Gem, that is, a converter, extension, etc. not just any Ruby component, so this kind of pre-initialization is not possible.

Unless I am wrong (/cc @robertpanzer), we will need to address this with a fix. I am thinking that adding every single option is not maintainable, the maven plugin would need to be updated on every change. But we could add a free <options> section similar to <attributes> so that you can set any options required. For instance sourcemap or catalog_assets. wdyt?

@abelsromero
Copy link
Member

I've thinking about this and there's one point to discuss: what to do with empty (<mkDirs></mkDirs>) and null tags (<mkDirs/>).
Right now all options have some value (http://asciidoctor.org/docs/user-manual/#ruby-api-options), so we could force the value for boolean types, this means that users must write true. But I think this is not user friendly and is confusing when you compare it with how attributes works. Note that some attributes are flags, not boolean.

So, I think we should allow for empty and null tags to be used as boolean true value.

@mojavelinux
Copy link
Member

I support requiring an explicit boolean value for options like mkdirs.

<mkdirs>true</mkdirs>

It's reasonable to have different requirements between options and attributes since they are distinct inputs into the API.

@robertpanzer
Copy link
Member

I'd also prefer the explicit style so that you could also put false in
there.

Am Montag, 12. September 2016 schrieb Dan Allen :

I support requiring an explicit boolean value for options like mkdirs.

true

It's reasonable to have different requirements between options and
attributes since they are distinct inputs into the API.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#264 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABHBjiYNRbiWVb3oKb3OtNd7ZyIWrBhQks5qpaH_gaJpZM4J6BhU
.

@mojavelinux
Copy link
Member

I 100% support allowing all the public options to be configured.

However, for the use case specifically cited in this issue, I think the extension should be responsible for enabling the sourcemap (rather than relying on the user to enable it for the extension's purpose). This can be done in a Preprocessor. The preprocessor should explicitly enable the sourcemap flag on the Document instance. I believe @robertpanzer recently added this to the the 1.6.0 branch.

@mojavelinux
Copy link
Member

@abelsromero can you make a checklist in this issue of the options you plan to map? Not all of them make sense in this context, so we should have a look at the list and make a decision about which ones to pick.

Btw, I also updated the documentation for the Ruby API options, including correcting the information about the timings option.

@abelsromero
Copy link
Member

My idea was to add a options Map similar to how attributes work, that way users can use all options. Honestly, I'd rather avoid adding validations, if that's the case we can just add individual attributes and deal with the options case-by-case. Right now we can add:

  • sourcemap
  • catalog_assets

@mojavelinux
Copy link
Member

Perhaps also add template_cache as well.

It's probably best to keep all the options together, so I recommend either moving all the options to an options map or add the new options to the top-level. Honestly, I'd support just adding the options at the top-level (adjacent to baseDir, backend, etc). That's kind of the structure we have now.

@abelsromero abelsromero added this to the 1.5.4 milestone Sep 13, 2016
@abelsromero
Copy link
Member

I like the "top-level" option because it makes the configuration easier and friendlier. But if I understood correctly, this means that whenever a new options is added into the core, it would need to be added to the plugin.

@mojavelinux
Copy link
Member

That's correct. And I think that's reasonable. New options are rarely added.

@abelsromero
Copy link
Member

👍
sourcemap, catalog_assets and template_cache it is. I'll get to it this afternoon.

@AlexanderZobkov
Copy link
Author

@abelsromero Thanks for fast feedback on the issue. I also support "top-level" option as some RubyAPI options are already there (for example: backend, doctype and header_footer).
I think the same issue needs to be submitted for gradle plugin to have the same level of capabilities.

@AlexanderZobkov
Copy link
Author

@mojavelinux, you mention that sourcemap option can be enabled by using a preprocessor. Could you please provide a snipped in Ruby how this can be achieved?

Unfortunately I can't use asciidoctorj API 1.6 as this version is not compatible with current asciidoctor-maven-plugin. It's problematic to use Treeprocessors based on Asciidoctorj API 1.5 due to several reasons discussed in other issues reported by the community. So that I'm writing my treeprocessor in Ruby. I've managed to enable Ruby based extension by leveraging "requires" option of the asciidoctor-maven-plugin.

abelsromero added a commit to abelsromero/asciidoctor-maven-plugin that referenced this issue Sep 13, 2016
abelsromero added a commit to abelsromero/asciidoctor-maven-plugin that referenced this issue Sep 13, 2016
@abelsromero
Copy link
Member

Hi @AlexanderZobkov, before merging the PR and closing this issue, I'd like to know if you could you find a solution? At least from Java, setting "source_map" in a Preprossessor did not work :/ I could only retrieve the information when setting the options before calling the render method.

@AlexanderZobkov
Copy link
Author

Hello @abelsromero , I did not have a chance to look into this since then, hope this weekend I will be able to do so. I'm not sure, as well, that Preprocessor (even Ruby based) allows such kind of manipulations or probably we don't know how to cook it 😄 My idea is to hack Asciidoctor Ruby API to set "source_map" option thanks to Ruby monkey patching capabilities.

@mojavelinux
Copy link
Member

From Ruby it's as simple as this:

require 'asciidoctor/extensions'

Asciidoctor::Extensions.register do
  preprocessor do
    process do |doc, reader|
      doc.instance_variable_set :@sourcemap, true
      nil
    end
  end
end

The instance_variable_set is required since the property currently lacks a setter method. I will change that for 1.5.5.

You can see the impact in the treeprocessor:

Asciidoctor::Extensions.register do
  treeprocessor do
    process do |doc|
      puts (doc.find_by context: :paragraph).last.source_location
    end
  end
end

@mojavelinux
Copy link
Member

@AlexanderZobkov
Copy link
Author

Thanks @mojavelinux it's working!

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

No branches or pull requests

4 participants