-
Notifications
You must be signed in to change notification settings - Fork 75
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
Process dataset elements with access children #122
Conversation
I'm guessing this will have some pep issues... |
124431a
to
91431f6
Compare
siphon/catalog.py
Outdated
# access_urls | ||
has_access_element_info = \ | ||
any(self.datasets[previous_dataset].access_element_info) | ||
if has_access_element_info: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be if dict...no need for any
siphon/catalog.py
Outdated
@@ -297,8 +328,26 @@ def make_access_urls(self, catalog_url, all_services, metadata=None): | |||
access_urls[service.service_type] = server_url + \ | |||
service.base + self.url_path | |||
|
|||
if any(self.access_element_info): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nuke this line - won't iterate over empty dict
siphon/catalog.py
Outdated
for service_type in self.access_element_info: | ||
url_path = self.access_element_info[service_type] | ||
found_service = None | ||
for service in all_services: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make all_services -> service.name use dict comprehension to clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reuse in line 317, too.
Looks like my quote-fixing spree is in conflict. Sorry, dude. |
Nah, that was an easy fix. I'll up updating the PR in the next few minutes. |
@dopplershift have you thought about turning off merge and squash+merge commits and just allowing the rebase and merge option? |
I've fallen out of favor with the rebase and merge button--I like seeing the split and merge for each PR. What I still want is the ability to rebase the branch from the web UI rather than the merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending CI finish.
More pep8. 😢 |
'Test parsing access elements in TDS client catalog' | ||
url = 'http://oceandata.sci.gsfc.nasa.gov/opendap/SeaWiFS/L3SMI/2001/001/catalog.xml' | ||
cat = TDSCatalog(url) | ||
assert len(list(cat.datasets)) != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh If we had only known and had written the test to check the correct length, which is 174.
Fix #114
Fix #115