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.

Sign up for GitHub

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

Jump to bottom

--search-dir is not expanded prior to passing to external module #56

Closed
unode opened this issue Jan 16, 2018 · 4 comments · Fixed by #89
Closed

--search-dir is not expanded prior to passing to external module #56

unode opened this issue Jan 16, 2018 · 4 comments · Fixed by #89
Labels
bug

Comments

@unode
Copy link
Member

unode commented Jan 16, 2018 •
edited
Loading 

external(file="<data>/input.fa")

called with ngless --search-dir data=/somedir results in calling external() with <data>/input.fa instead of /somedir/input.fa.

Testcase here
@luispedro luispedro added the bug label Feb 20, 2018
@unode
Copy link
Member Author

unode commented Mar 14, 2018 •
edited
Loading

Possibly related, if a path doesn't exist an error:

Line X: Could not find necessary input file <references>/path/to/file

is shown. It might be useful to also display the resolved path in the error.

When I first saw this I wasn't sure if I did a mistake specifying --search-dir or a typo on /path/to/file. Turned out to be neither and the file was simply missing.

@unode
Copy link
Member Author

unode commented May 7, 2018

Now that we have a few external modules we should fix this before next release.

unode added a commit that referenced this issue May 12, 2018
@unode
BUG Fix expansion of searchdir with external modules
6e8ef2b 
This is an incomplete fix as it only covers the successful cases.
If a file does not exist in the specified location no expansion happens.

refs #56
unode added a commit that referenced this issue May 12, 2018
@unode
BUG Fix expansion of searchdir with external modules
42b2cd0 
This is an incomplete fix as it only covers the successful cases.
If a file does not exist in the specified location no expansion happens.

refs #56
unode added a commit that referenced this issue May 15, 2018
@unode
TST+BUG Slightly better solution to #56
9db3fb2 
Previous solution only applied to unnamed arguments
@unode
Copy link
Member Author

unode commented May 15, 2018

This is still not complete as the current implementation doesn't fail if it was not possible to resolve search-dir.

luispedro added a commit that referenced this issue Jun 5, 2018
@luispedro
RLS Release v0.8.1
f292611 
Bugfix release. Trigger for release was the updated conduit-algorithms
as it makes a huge difference:

    Version 0.8.1 2018-06-05 by luispedro
        * Update to LTS-11.12 (for faster conduit-algorithms used in collect())
        * Add fallback for character encoding on systems with bad locale support
        * Fixed lock1 when used with paths (#68)
        * Fixed expansion of searchdir with external modules (#56)
luispedro added a commit that referenced this issue Jun 5, 2018
@luispedro
RLS Release v0.8.1
d066fbb 
Bugfix release. Trigger for release was the updated conduit-algorithms
as it makes a huge difference:

    Version 0.8.1 2018-06-05 by luispedro
        * Update to LTS-11.12 (for faster conduit-algorithms used in collect())
        * Add fallback for character encoding on systems with bad locale support
        * Fixed lock1 when used with paths (#68)
        * Fixed expansion of searchdir with external modules (#56)
@unode
Copy link
Member Author

unode commented Jun 13, 2018

On offline discussions we realized the current approach doesn't allow distinguishing between a file path and an string argument to be passed as-is.

All solutions discussed would require changes on the API of external modules.
Some of the alternatives mentioned:

  • Add atype=path to distinguish from atype=str - path would be searchdir expanded while str reverts to the old behavior and wouldn't be expanded.
  • Add a expand_searchdir=true attribute that explicitly specifies which API arguments should be searchdir expanded.

unode added a commit to unode/ngless that referenced this issue Oct 20, 2018
@unode
ENH+TST Make search-dir expansion explicit in external modules
f33b778 
closes ngless-toolkit#56
unode added a commit to unode/ngless that referenced this issue Oct 20, 2018
@unode
ENH+DOC+TST Make search-dir expansion explicit in external modules
dde9098 
closes ngless-toolkit#56
@unode unode mentioned this issue Oct 20, 2018
Merged
@unode unode closed this as completed in #89 Oct 22, 2018
unode added a commit that referenced this issue Oct 22, 2018
@unode
ENH+DOC+TST Make search-dir expansion explicit in external modules
9769563 
closes #56
luispedro added a commit that referenced this issue Nov 12, 2018
@luispedro
BLD Release 0.10
af3b5f8 
Many small fixes rather than any large new features.

Full ChangeLog:

    * Fix to lock1's return value when used with paths (#68 - reopen)
    * Support _F/_R suffixes for forward/reverse in load_mocat_sample
    * samtools_sort() now accepts by={name} to sort by read name
    * Fixed bug where header was printed even when STDOUT was used
    * Fixed bug where writing interleaved FastQ to STDOUT did not work as
    expected
    * Indices created by bwa and minimap2 are now versioned
    * arg1 in external modules is no longer always treated as a path
    * Added expand_searchpath to external modules API (closes #56)
    * Fixed bug where detection of Fastq encoding was not performed on the second pair
    * Fix saving fastq sets with --subsample (issue #85)
    * Add __extra_megahit_args to assemble() (issue #86)
    * Better error message when user mis-specifies the ngless version string
    (issue #84)
    * Support NO_COLOR environment variable (issue #83)
    * Garbage collection for temporary files (issue #79)
    * Rename --search-dir to --search-path for consistency with other API
    * Fix corner case with select() producing incorrect CIGAR strings (#92)
    * Always check output file writability (#91)
    * Make paired() accept encoding argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Search-dir expansion with external modules unode/ngless
2 participants
@luispedro @unode