ngless-toolkit / ngless

NGLess: NGS with less work
https://ngless.embl.de
Other
142 stars 24 forks source link

lock1 fails if provided string contains folders #68

Closed unode closed 6 years ago

unode commented 6 years ago
samples = [
   "projectA/sampleA1",
   "projectB/sampleB1"
]
lock1(samples)

errors with:

[Sat 05-05-2018 17:07:20] Line 13: Interpreting [assignment]: lock1(Lookup 'samples' as NGList NGLString; __hash="d4531bdab467a3f1feb67e5ae87de081")
[Sat 05-05-2018 17:07:20] Line 13: Interpreting [executing module function: 'lock1']: NGOList [NGOString "projectA/sampleA1",NGOString "projectB/sampleB1"]
[Sat 05-05-2018 17:07:20] Line 13: Looking for a lock in ngless-locks/d4531bda. Total number of elements is 2 (not locked: 2; not finished: 2).
Exiting after internal error. If you can reproduce this issue, please run your script with the --trace flag and report a bug at http://github.com/luispedro/ngless/issues
ngless-locks/d4531bda/projectA/sampleA1.lock: lock: does not exist (No such file or directory)
unode commented 6 years ago

A simple workaround could be to replace any / or \ (windows) in the string by _.

unode commented 6 years ago

@luispedro Realized one use-case of this code that got broken with the above fix.

file_with_full_paths.txt

/path/to/file1
/another/path/to/file2
samples = readlines('file_with_full_paths.txt')
sample = lock1(samples)

sample now contains _path_to_file1 which follows the fix above makes the variable sample useless for users. Do you see any problem in returning /path/to/file1 from lock1() regardless of the name used for the lock?

luispedro commented 6 years ago

Yeah, that's how it should work.

unode commented 6 years ago

Fix in in place. Only missing a test-case to avoid future regressions. Not sure how to go about testing this thought.

luispedro commented 6 years ago

Thanks! This should be mentioned in the ChangeLog and What's New page for 1.0

Test could be:

ngless "0.9"
print(lock1(['testing/slashes\\ and spaces']))
unode commented 6 years ago

That last was already https://github.com/ngless-toolkit/ngless/blob/master/tests/parallel_folder_lock/lock.ngl minus the print(). Now it tests this too.

luispedro commented 6 years ago

The idea of adding print is that you can test whether the result is correct by having a output.stdout.txt file.

unode commented 6 years ago

Yup: https://github.com/ngless-toolkit/ngless/commit/2eff0333f595d472a2aa0bc85bb5f39066c3626b - What I was missing is that the tests run with --quiet.