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

uri starting with file:// were not interpreted correctly #1874

Merged
merged 2 commits into from Mar 17, 2016
Merged

Conversation

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Mar 9, 2016

fix #1867
@aashish24 @danlipsa @chaosphere2112 quick review

@@ -283,9 +283,10 @@ def openDataset(uri,mode='r',template=None,dods=1,dpath=None, hostObj=None):
uri = string.strip(uri)
(scheme,netloc,path,parameters,query,fragment)=urlparse.urlparse(uri)
if scheme in ('','file'):
if uri[:7].lower() == "file://":
Copy link
Contributor

@aashish24 aashish24 Mar 9, 2016

python does not have method load from file:// directly?

Copy link
Contributor

@chaosphere2112 chaosphere2112 Mar 9, 2016

@aashish24 Nope, Python doesn't support arbitrary open on URLs like PHP.

Copy link
Contributor

@aashish24 aashish24 Mar 9, 2016

@chaosphere2112 I thouught that urllib would have something .. but this code is fine.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Mar 9, 2016

@aashish24 first off it's cdms open not python open.
Second we are using:
(scheme,netloc,path,parameters,query,fragment)=urlparse.urlparse(uri)
but if it starts with file:// then path and root are both empty strings.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Mar 9, 2016

@chaosphere2112 I'm not 100% sure how the file:// got appended to the uri in the first place... I have a vague recollection of you asking me to do this forthe GUI so I'll take the blame. But it might be worth tracing down.

Copy link
Contributor

@aashish24 aashish24 Mar 9, 2016

but if it starts with file:// then path and root are both empty strings.

@doutriaux1 path should not be empty if you parse it via urlparse. I just did a local test to confirm this.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Mar 9, 2016

LGTM but will wait for travis to finish.

@chaosphere2112
Copy link
Contributor

@chaosphere2112 chaosphere2112 commented Mar 9, 2016

@doutriaux1 @aashish24 Just pushed a commit that should help solidify this a bit. I tested with all kinds of gnarly paths ("file://../../blah/test.nc", for example), which works. I also made it so we convert paths to absolute paths in the CdmsFile constructor before converting them to file URIs.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Mar 10, 2016

@chaosphere2112 can you add a test as well?

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Mar 10, 2016

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Mar 17, 2016

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Mar 17, 2016

@doutriaux1 i pushed again to resolve the conflict.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Mar 17, 2016

I am running it manually as well.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Mar 17, 2016

LGTM 👍 and worked on my machine as well. merging.

aashish24 added a commit that referenced this issue Mar 17, 2016
uri starting with file:// were not interpreted correctly
@aashish24 aashish24 merged commit 65ca26c into master Mar 17, 2016
0 of 2 checks passed
@aashish24 aashish24 deleted the issue_1867 branch Mar 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants