require_relative of "salt" in asciidoctor-diagram.rb

[ManDay]

I'm trying to patch asciidoctor-diagram to only install the diagram types which are requested through and supported by the package manager on Gentoo. What I did so far is delete the according directory and .rb file in /lib/asciidoctor-diagram, the spec-file, and the require_relative in asciidoctor-diagram.rb.

What doesn't play well with that logic is the mention of "salt" in the requires, because it isn't a diagram-type per se and only a requirement of PlantUML, but treated like a (unconditional) dependency of asciidoctor-diagram.

Could the require of "salt" be moved from the top level of asciidoctor-diagram into the PlantUML specific files?

[pepijnve]

The location of those files is part of the public API of the gem. Moving them around would be a backwards incompatible change. Maybe one day in a 3.0 release.

[mojavelinux]

From a bystander perspective, it seems like moving this require would be pretty safe since it is specific to a diagram type. One idea would be to change it, see if anyone reports it as a problem, and be ready to revert it if so. Just an idea.

[pepijnve]

The documentation states you can either require asciidoctor-diagram to enable all diagram types or asciidoctor-diagram/<type> to enable a specific one. Not sure how I can move the file without breaking that.

The alternative solution is that the split script that's being developed deals with this special case. There's another special case with structurizr which uses code from other extensions to do the actual rendering.

[mojavelinux]

Disregard my comment. I looked at the main script and now I see what you mean. From the report, I thought that it was requiring something globally it wasn't using. That's not the case. It is requiring every diagram extension as it should. You are right that if you want individual a la carte extensions, then you need to require them one-by-one.

[ManDay]

The alternative solution is that the split script that's being developed deals with this special case.

Yes, that's how I currently have it. The according bug and the patch/script can be seen here.

Not sure how I can move the file without breaking that.

I do not understand which file you are referring to. I did not mean to suggest that any file be moved. Only that the line

asciidoctor-diagram/lib/asciidoctor-diagram.rb

Line 23 in ac80bab

require_relative 'asciidoctor-diagram/salt'

is moved from the top-level asciidoctor-diagram.rb into one of the PlantUML specific files (in the sense that asciidoctor-diagram doesn't require it at the top level, but only for PlantUML).

[pepijnve]

@mojavelinux the main issue I have with this request and the discussion in #376 is that I'm not sure what this would actually solve. All of the extensions can be required even if the tools they depend on are not present on the system. It's only when a document actually uses a block type that you will get an error stating that some tool is missing.

It's perfectly feasible to split the gem into one per diagram type, but that means I now have to release a cluster of extensions instead of just one. The overhead of managing all those release numbers is what I want to avoid because the benefit seems rather limited.

[ManDay]

@pepijnve for the record, I don't mean to exaggerate the problem and I accepted your decision in #376. All I'm trying to achieve right now is a bit of clean-up for those who care about not having unnecessary files installed. Most people probably don't care about it, but if I can strike a balance between a sufficiently clean/low-effort downstream patch which improves the situation and effectiveness, it would be a nice-to-have.

I just suspected said line is accidentally misplaced in the top-level file and, if moved into the right place, would also help me with the patch. That's all. Feel free to close if you don't see any merit. The additional if in the patch wont cause any harm.

PS: Unrelated to this issue, but w.r.t. #376, if there were something like "compile time" configuration, then it would make a natural solution for either installing or not-installing different document-types, without releasing them all separately. My downstream patch is pretty much that: A poor-man's ./configure to enable/disable different features.

[mojavelinux]

All of the extensions can be required even if the tools they depend on are not present on the system. It's only when a document actually uses a block type that you will get an error stating that some tool is missing.

That's a reasonable design/assumption from my perspective. I would continue to focus on allowing the path to the tools be controlled using variables (AsciiDoc attributes or environment variables). If the tool cannot be found, logging a warning (or error) is the right way to communicate the requirement IMO.

It's perfectly feasible to split the gem into one per diagram type, but that means I now have to release a cluster of extensions instead of just one.

I would not like that either, and I doubt users would. It's just unnecessary abstraction. It's better to be pluggable than to be modular.

[mojavelinux]

moved from the top-level asciidoctor-diagram.rb into one of the PlantUML specific files

If the salt diagram type ends up requiring the plantuml diagram type (which it appear to), then I suppose it could be moved around so that requiring the plantuml diagram type also requires the salt diagram type. However, if there's a need to require plantuml without requiring salt, then you have just introduced the opposite problem.

[pepijnve]

I just suspected said line is accidentally misplaced

I'm going to close this issue since the assumption was that this was accidental, while it is not. The convention is that requiring asciidoctor-diagram/<diag-type> enables the <diag-type> block. That convention holds for salt. What is, in retrospect, an accident is that blockdiag enables a bunch of additional block types as well. Those should have been split out as well.