Fix collection option related issue #286
Conversation
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
abunashir
force-pushed
the
fix-collection-output-path
branch
from
9e24967
to
e8d4b70
Compare
lib/metanorma/cli/collection.rb
Outdated
| @@ -26,8 +27,14 @@ def collection_file | |||
| @collection_file ||= Metanorma::Collection.parse(file) | |||
| end | |||
|
|
|||
| def verify_required_options_are_present | |||
| if collection_options[:output_folder].nil? | |||
| raise ArgumentError.new("Missing a required option '--output-folder'") | |||
There was a problem hiding this comment.
We shoud provide a default path for "output folder" just like how metanorma compile sources/sample.adoc uses the location of the input file as the output folder:
metanorma compile sources/sample.adoc=>sources/sample.xmloutputmetanorma collection sources/collection.yaml=>sources/collection.xmloutput
There was a problem hiding this comment.
That's a good point to set the source path as a default output folder 👍🏼
Actually, after looking into it I came across a bit tricky issue - looks like we are recreating the output folder before we write the outputs, so if we use the source directory as the output path then it gets removed (along with the source file) in the recreation phase - here is the source code.
So, what do you guys think about using ...source/output as a default output directory if nothing was provided?
// @ronaldtse, @opoudjis
There was a problem hiding this comment.
That code is clearly wrong, we should NOT be removing the output directory in any case.
We want to use the source path as the default output folder (i.e. my_source/x.yml => my_source/x.{outputformats}). Unless I'm missing something here...?
There was a problem hiding this comment.
I'm not sure about the exact reasoning whey we need to remove the output directory before writing the output. @andrew2net, @opoudjis - Do you guys have any idea why we are doing that, do we still need to keep that logic there?
There was a problem hiding this comment.
I would have done it to ensure that only data from the current run was present in the folder. Arguably that's overly cautious. Maybe. Ok, get rid of it.
There was a problem hiding this comment.
Cool, I just created a PR for that - metanorma/metanorma#259, once that's merged and released then we can merge this one. FYI, I will update this PR to use the source folder next :)
abunashir
force-pushed
the
fix-collection-output-path
branch
2 times, most recently
from
da08dcc
to
65a5ace
Compare
Currently, the collection is re-creating the output directory, and it might causes issues like removing all files from the output folder. Normally this is okay, but it become an issue where the user is using the source path as output directory and it deletes all source files. This commit changes that, and only tries to create a folder when the output folder doesn't already exists. Related: metanorma/metanorma-cli#286
We have recently added support to provide collection options either as an command line arguments or options in the collection file. But that is only working on the site interface, and the collection cli still expect the `output-folder` as required option. This commit changes that, so it's not required anymore and it will try to read the option from the file, and if none of those source has this value, then it will use the source as output directory. Fixes #283
abunashir
force-pushed
the
fix-collection-output-path
branch
from
f829c4e
to
4636428
Compare
|
I just updated the PR to use the source file's folder as default output location for collection. Once that is merged and released - metanorma/metanorma#259, please feel free to merge this one :) //cc: @ronaldtse, @opoudjis |
abunashir
dismissed
ronaldtse’s stale review
Can you have another look please?
We have recently added support to provide collection options either as a command line arguments or options in collection file. But that is only working on the site interface, and the collection cli still expect the
output-folderas required option.This commit changes that, so it's not required anymore and it will try to read the option from the file, and if none of those source has this value, then it will use the source as output directory.
Fixes #283