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

CLI should set :mkdirs option by default #1241

Closed
mojavelinux opened this issue Nov 16, 2023 · 7 comments
Closed

CLI should set :mkdirs option by default #1241

mojavelinux opened this issue Nov 16, 2023 · 7 comments

Comments

@mojavelinux
Copy link
Member

To be consistent with the CLI from the asciidoctor gem (asciidoctor), the AsciidoctorJ CLI should set the mkdirs option by default. Enabling this option allows the output file to be written to a directory which does not yet exist. The CLI has no other way of controlling this behavior, so the option should be on by default.

Right now, the following example will fail:

asciidoctorj -o /tmp/does/not/yet/exist/doc.html doc.adoc

However, the same command will succeed when using asciidoctor.

Here's the code in the Asciidoctor CLI that enables this option: https://github.com/asciidoctor/asciidoctor/blob/fd0e92a78b18e974a567b0ef798d99bbbb8c6acc/lib/asciidoctor/cli/invoker.rb#L96-L104

@robertpanzer
Copy link
Member

Could this be a duplicate of #1135?
It should be fixed on the main branch (#1137).
For now I considered this a breaking change and therefore did not pull it into the 2.5.x line.
Do you think it would make more sense to classify this as a bug fix and pull it into the next 2.5.x release?

@mojavelinux
Copy link
Member Author

Yep, that looks like the same issue. Since :mkdirs wasn't mentioned, it wasn't coming up in my search of the issue tracker or CHANGELOG.

I definitely consider this to be a bug, not a breaking change. For one, the current behavior doesn't match the behavior of asciidoctor, so there's an expectation that it should work. To me, fixing that discrepancy is not a breaking change. If anything, it will unbreak things.

Speaking of which, this issue is preventing AsciidoctorJ from being used with the Antora PDF extension, which requires the command to be able to write to a directory that doesn't yet exist. On top of that, most applications I know of will create missing directories when writing to a file (for example, the libreoffice command). Interesting to note that pandoc will not, but that seems like an outlier.

My recommendation is to move forward with putting this fix in the 2.5.x line.

robertpanzer added a commit to robertpanzer/asciidoctorj that referenced this issue Nov 19, 2023
… should be relative to destination dir if source dir is given.
@robertpanzer
Copy link
Member

I created #1242 with a fix on the v2.5.x.
Is it possible that you could test if it works as expected?

If possible, could you also check if the breaking changes in the current changelog of the main branch are really breaking changes or should be backported to v2.5.x? (Obviously I need to update it to move this bug fix out of the breaking changes).
Tbh the classification into non-breaking and breaking changes is stressing me out, I probably had too many such discussions at work... (And then there are discussions if breaking changes are possible at all when it comes to the extension API)

@mojavelinux
Copy link
Member Author

I created #1242 with a fix on the v2.5.x.

Thanks!

Tbh the classification into non-breaking and breaking changes is stressing me out, I probably had too many such discussions at work

I try not to overthink it. The way I look at it, if it requires users to change their code to avoid their code/logic from breaking, that's a breaking change. If we are fixing something that should have worked, that's not a breaking change, which is the case here. What this change does is allows something that didn't work before to start working. It's impossible (read as: extremely unlikely) that will break existing logic.

The only non-breaking change that is listed as a breaking change is this one:

Allow Preprocessor extensions to create new Readers and replace the original Reader.

Unless that change causes a compilation change, I don't see how it's breaking.

Technically, changing the -s option to -e is a bug fix, but since we can consider the AsciidoctorJ CLI to be independent, I think that is something that could cause an application to break.

@robertpanzer
Copy link
Member

The only non-breaking change that is listed as a breaking change is this one:

Allow Preprocessor extensions to create new Readers and replace the original Reader.

That is an API change in the extension API, so it requires even a code change for existing extensions.

@robertpanzer
Copy link
Member

Thanks for going through the change log! ❤️

robertpanzer added a commit to robertpanzer/asciidoctorj that referenced this issue Nov 20, 2023
robertpanzer added a commit that referenced this issue Nov 23, 2023
* CLI should set :mkdirs option by default (#1241)

* Simplify test by avoiding use of wildcards
@robertpanzer robertpanzer mentioned this issue Dec 2, 2023
5 tasks
@robertpanzer
Copy link
Member

Setting the option :mkdirs should be fixed in 2.5.11.
Additionally also handling creating subdirectories correctly in the destination path for files that are in a nested hierarchy relative to the source dir is fixed in 3.0.0-alpha.2.

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

No branches or pull requests

2 participants