issue_comments

@juliancanellas Great! Good to see that someone else actually benefits from this feature, years after it was implemented 😄

I got the same error when doing ds = xr.open_dataset(url, engine='pydap') using xarray 0.10.0, but when i install directly from master i don't get the error. So it might have been fixed already.

Thank you too! Cool :)

This looks great to me!

One last suggestion -- can you add a brief note about the ability to access password protected datasets in the opendap section of the docs? http://xarray.pydata.org/en/stable/io.html#opendap

Thanks! It's cool to be able to contribute to a library i use daily.

I added some documentation, please tell me what you think. For the second example, with the NASA URS, i wanted to use a URL that points to an actual NASA dataset, but i couldn't find one that wasn't extremely long. I ended up using that one, which is for a real NASA server, but a non-existant dataset. Hope that's not too confusing. Obviously, the username and password are fake as well :) so maybe it doesn't matter.

I fixed the documentation stuff and changed the test to one that uses a mock of pydap.client.open_url, like we talked about.

I decided against adding open_urls other keyword arguments to PydapDataStore.open, for the reasons that we discussed and since users can still instantiate their own Pydap dataset and pass that, if they really want to change those parameters.

Thank you for the fast review!

but if there are a few other optional keyword arguments to complete the function signature, then sure, that would be fine. I would definitely avoid using **kwargs, because that precludes the ability for us to add other xarray specific options at some point in the future.

One of the reasons i was unsure about adding the other keyword arguments, is that I'm not entirely sure if changing any of them from their default values will break the interface with xarray. But i guess you could argue that if users are changing these parameters, they probably know what they are doing.

Then there's also the timeout parameter, for which I think we will need to import some module within pydap just to get DEFAULT_TIMEOUT. That seems a bit awkward? We could also just leave that one out.

I definitely see your point with **kwargs, I had a feeling that would be a bad idea.

Thank you @shoyer, I'll start work on the implementation.

@shoyer @jenfly Has this been implemented? I can't see any open PRs relating to this, so I guess no one is working on it?

I would be happy to try and implement it, if that's fine with you? It seems like you settled on the solution of passing a session object to a PydapDataStore and then passing that to open_dataset(), correct?

Thanks in advance!