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.

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

Resolve asset spec as path if a file exists with that path #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

viningM
Copy link

@viningM viningM commented Sep 15, 2016

Fixed the issue described here: #57
For file paths on windows, the path was treated as a package due to having a colon in it.

With the fix, any asset spec will first be checked to see if it is a local file path before being treated as a package.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 17, 2016

Excellent! It's been a while since I looked at this repo, but this looks good. It would be nice to make sure it's covered by the test suite. If you can add a test, that'd be great. I'll take a look over the weekend and see if I can recommend any way to test it if it's not obvious to you.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 19, 2016

Can you fix a couple things before I merge this, please?

  1. We should return (None, item) rather than (__name__, item) because that's the signal that this string is not an asset spec and has no package part. When this happens, depending on why we were checking, we do things that this PR might break, such as calling the parent class to check relative paths in all the configured asset directories.

  2. Sometimes, the file may not exist but it's still meant to be treated as a file. I'm not positive, but this could be the case if, for example, an absolute Windows file path is used as an output path. Instead of checking "isfile", I think we should check whether the start of the path ismount().

@viningM
Copy link
Author

viningM commented Sep 20, 2016

The first change you request results in a type error, which is why I went with name in the first place, as a hack-y workaround. I'm not sure how to avoid this.

The second change is implemented and pushed.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 91.284% when pulling 03fcc1a on viningM:master into 060b532 on sontek:master.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 20, 2016

Hmm, there should be no type error. Other code paths return None as the first of that tuple, but I'll look into it.

Looking at your patch now, it's still not quite right. The ismount has to be called on the mount point, I think. I'm not on a windows machine right now, but I think ismount would only succeed for something like "C:" or "C:" or UNC mounts. The whole item is string, such as "package:path/to/asset" or "C:\static\path\to\asset", will not return True for ismount.

@viningM
Copy link
Author

viningM commented Sep 20, 2016

Unfortunately I'm out of time to poke at it, I'll leave to you to fix or ignore.

Best of luck

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants