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

Avoid ambigous destination value in test case... #186

Merged
merged 2 commits into from May 7, 2015

Conversation

Projects
None yet
2 participants
@peterjc
Copy link
Contributor

peterjc commented May 7, 2015

Suggestion: If the source has any wildcards (or is a folder with an implicit wildcard), then destination must be a directory given with trailing slash (otherwise error).

The include source of "../shared_files/extra_test_data/**" would only find "../shared_files/extra_test_data/extra_test_file.txt" which after strip_components 3 gives "extra_test_file.txt"

Question: did the ".shed.yml" destination of "test-data" mean call this file "test-data" [sic] or use "test-data/extra_test_file.txt" instead? Why?

What if the include source glob matches multiple files? Then might expect destination must be a folder - since alternative means trying to map multiple input files to one in the tar-ball/

Using destination "test-data/" seem unambiguous and preferable.

(I found this issue during re-factoring while working on #180)

Change ambigous test case...
The include source of "../shared_files/extra_test_data/**" would
only find "../shared_files/extra_test_data/extra_test_file.txt"
which after strip_components 3 gives "extra_test_file.txt"

Question: did the ".shed.yml" destination mean call this file
"test-data" [sic] or use "test-data/extra_test_file.txt" instead?

What if the include source glob matches multiple files? Then might
expect destination must be a folder?

Using destination "test-data/" seem unambiguous.
@peterjc

This comment has been minimized.

Copy link
Contributor Author

peterjc commented May 7, 2015

P.S. I am willing to try and add code to catch the proposed error condition.

@jmchilton

This comment has been minimized.

Copy link

jmchilton commented on 9325736 May 7, 2015

What if the include source glob matches multiple files? Then might
expect destination must be a folder?

Yes - I think that I make the assumption in the code that if the glob matches more then one file the destination must be a directory. The test case was my verification that this was working without needing the /. Too much magic?

This comment has been minimized.

Copy link
Owner Author

peterjc replied May 7, 2015

I think that's too much magic, yes. In this case the wildcard turns out to mean one file - leaving the situation ambiguous.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented May 7, 2015

Okay - I can live with that - what you are proposing seems like a tighter, more reasonable spec. We should make sure shed_lint provides a good description of the problem though.

@peterjc

This comment has been minimized.

Copy link
Contributor Author

peterjc commented May 7, 2015

So changes to both shed_lint (new ground for me) and shed_upload (starting to understad) needed?

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented May 7, 2015

If you return an exception - like unmatched globs - I think shed_lint will just print a message for it - so we should verify this but I don't think it will require changes to shed_lint.

Update: This line https://github.com/galaxyproject/planemo/blob/master/planemo/shed/__init__.py#L775 is what I was referring to about "returning" an exception.

I can definitely work on that part - the concept of "missing" needs to be generalized to other sorts of validation exceptions. But as long as we get them out of that chunk of code as exceptions shed lint should handle it.

jmchilton added a commit that referenced this pull request May 7, 2015

Merge pull request #186 from peterjc/ambiguous_destination
Avoid ambigous destination value in test case...

@jmchilton jmchilton merged commit 8f23885 into galaxyproject:master May 7, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented May 7, 2015

Awesome - thanks for working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment