Fixes #1135 on v2.5.x (#1241). Target file name should be relative to… #1242
Conversation
… should be relative to destination dir if source dir is given.
| @@ -302,7 +303,7 @@ public Options parse() { | |||
| } | |||
|
|
|||
| if (isBaseDirOption()) { | |||
| optionsBuilder.baseDir(new File(this.baseDir)); | |||
| optionsBuilder.baseDir(new File(this.baseDir).getCanonicalFile()); | |||
There was a problem hiding this comment.
Is adding getCanonicalFile() related to this change?
There was a problem hiding this comment.
I added getCanonicalFile() to the other options to fix an issue with -D <target>:
If I start with this file layout:
├── sub1
├── sunset.jpg
└── test.adoc
and run the following command from the parent of sub1 like this:
asciidoctorj -r asciidoctor-diagram -R . -D target '**/*.adoc' -v
then I end up with this without .getCanonicalFile():
├── sub1
│ ├── sunset.jpg
│ └── test.adoc
└── target
├── test.html
└── test.svg
This seems wrong to me.
With .getCanonicalFile() I get this:
├── sub1
│ ├── sunset.jpg
│ └── test.adoc
└── target
└── sub1
├── test.html
└── test.svg
I added the call to .getCanonicalFile() to baseDir to have the code symmetric.
Could this create any issue?
There was a problem hiding this comment.
I think there are two concerns here. First, I'd say that this is a separate issue and thus should be in a separate PR. The next is whether this matches the behavior of Asciidoctor's CLI. I don't think the AsciidoctorJ CLI should be adding functionality the Asciidoctor CLI doesn't have, especially when we are talking about the same option flags. If the change does make it work like the Asciidoctor CLI, then I would agree it's the right bug fix (but still in a separate PR).
There was a problem hiding this comment.
#1135 complained about that difference in behavior.
Fixing the rendering into the nested directory structure required setting the mkdirs option.
While fixing that bug at the time I copied the logic from Asciidoctor.
I can't fully remember everything I was looking at while I was fixing this, but I remember that I had to look up some things in path_resolver.rb.
If you think that we should remove converting into subdirectories I can remove that again and only set the mkdirs option. I wonder though if this isn't exactly what you need, create subdirectories and convert the files into these subdirectories.
There was a problem hiding this comment.
If it were me, I would probably make two separate PRs. But since they are so closely related, I can at least understand why only one might be needed in this case.
There was a problem hiding this comment.
I can do that if you prefer.
There was a problem hiding this comment.
Regarding the getCanonicalFile() I did some more puts debugging.
It seems like Asciidoctor Ruby does sth similar for the source_dir here:
https://github.com/asciidoctor/asciidoctor/blob/f1acb3dafbcb7834cc0e6e00f8634f609ff42402/lib/asciidoctor/cli/invoker.rb#L57
AsciidoctorJ does not run this code, so it has to canonicalize it itself.
(Ruby's File.expand_path() and also File.absolute_path() seem to return the canonical path instead of the simpler absolute path so that I would need to use .getCanonicalFile() instead of .getAbsoluteFile().
But my comment refers to the sourceDir, while your comment refers to the baseDir.
I added the call to .getCanonicalFile() to the other options to have the code coherent.
Looking at the Asciidoctor code, it seems like all use sites also compute the canonical path, for example:
https://github.com/asciidoctor/asciidoctor/blob/f1acb3dafbcb7834cc0e6e00f8634f609ff42402/lib/asciidoctor/convert.rb#L85
https://github.com/asciidoctor/asciidoctor/blob/f1acb3dafbcb7834cc0e6e00f8634f609ff42402/lib/asciidoctor/document.rb#L393-L394
Looking at how the parameters look like exactly that are passed to Asciidoctor::convert I see now there are some differences, for example I wan't really aware that :source_dir doesn't seem to be passed to Asciidoctor::convert at all. I probably mixed up options with opts in invoker.rb.
I'll redo the entire thing once I find the time, that's certainly a blocker for releasing 3.0.0.
|
As discussed will open 2 new PRs with separate changes for setting the mkdirs option and fixing the conversion into these created directories. |
…tion dir if source dir is given.
Thank you for opening a pull request and contributing to AsciidoctorJ!
Please take a bit of time giving some details about your pull request:
Kind of change
Description
What is the goal of this pull request?
This PR fixes the behavior of the CLI to automatically set the mkdirs option similar to how the asciidoctor command does it.
This was originally reported in #1137 which was fixed on the main branch for the next version 3.0.0 of AsciidoctorJ.
Since it got erroneously classified as a breaking change this PR fixes the issue on the v2.5.x branch as well.
How does it achieve that?
Are there any alternative ways to implement this?
Are there any implications of this pull request? Anything a user must know?
Issue
If this PR fixes an open issue, please add a line of the form:
Fixes #1241
Release notes
Please add a corresponding entry to the file CHANGELOG.adoc