geostyler / geostyler-cli

BSD 2-Clause "Simplified" License
22 stars 8 forks source link

Conversion to an output file writes to a directory instead of a file #344

Closed geographika closed 1 month ago

geographika commented 1 year ago

This issue is linked to changes in https://github.com/geostyler/geostyler-cli/pull/335 The code below checks if the output parameter exists and if it is a file, however when writing an output this file won't exist, and targetIsFile is always false.

  // Try to define type of target (file or dir).
  let targetIsFile = false;
  const outputExists = existsSync(outputPath);
  if (outputExists) {
    targetIsFile = lstatSync(outputPath).isFile();
  }

This leads to the output being written to subdirectories based on the input e.g.

npm start -- -s qgis -t qgis -o output.sld testdata/point_simple.qml creates output.qml/point_simplepoint.qml

The workflow in https://github.com/geostyler/geostyler-cli/pull/343 highlights this issue. See for example the output at https://github.com/geographika/geostyler-cli/actions/runs/4925040568/jobs/8798798475#step:8:13

I'm unsure of the best approach to fix this. Would simply checking for an extension of the output parameter to set targetIsFile be enough?

KaiVolland commented 1 year ago

If an output is specified it should always "win". I guess i just missed to test this enough. So if you are able to fix this please go ahead.

geographika commented 1 year ago

@KaiVolland - the issue is the output could be specified as a file or folder. As they might not yet exist I can only think of checking for a file extension in the-o parameter. A folder could contain . though e.g./tmp/09.05.2023/folder Maybe we could require the specified output root folder to exist? It would also be good to add a test to https://github.com/geostyler/geostyler-cli/pull/343 for processing a folder. Should I add some test files for this?

KaiVolland commented 1 year ago

We could also check if the -o parameter contains at least one path.sep (/) if not we assume it is a file. And if you want a folder to be create at root level it has to start with ./?

geographika commented 1 year ago

We could also check if the -o parameter contains at least one path.sep (/) if not we assume it is a file. And if you want a folder to be create at root level it has to start with ./?

A common case could be to put a path to a file e.g. /mystyles/output.json', and on Windows a folder could beC:\Temp`. I can't think of a consistent way of telling if a string is a path to a folder or file which doesn't yet exist.

Attempted fix at #345 but it relies on the root folder being created first.

Maybe checking if the source is a folder (which has to exist) would be a better option? If a user put a path to a file in the output parameter then it would simply create a folder with a "file-like" name.

jansule commented 1 month ago

@geographika is this issue solved?

geographika commented 1 month ago

@jansule - yes fixed in https://github.com/geostyler/geostyler-cli/pull/347/files. Closing.

  // Assume the target is the same as the source
  let targetIsFile = sourceIsFile;