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

Catalog strptime #272

Merged
merged 11 commits into from
Aug 13, 2019
Merged

Catalog strptime #272

merged 11 commits into from
Aug 13, 2019

Conversation

deeplycloudy
Copy link
Contributor

Some datasets, including various GOES datasets on the Unidata THREDDS catalog, don't have a time dimension, but that time dimension can be inferred from the filename. However, those GOES filenames have julian days in the timestamps, making it not possible to trivially parse them with the current regex approach in DatasetCollection.

This PR adds an additional regex tag strptime and an additional argument for supplying the datetime.strptime format string. This functionality increases the number of filenames from which time information can be harvested .

@zbruick
Copy link
Contributor

zbruick commented Jul 15, 2019

Looks like there are some flake8 issues and we probably need a test added for this as well.

@deeplycloudy
Copy link
Contributor Author

It looks like there is no existing test for regex= with filter_time_*, so do we really need two tests — one for the old behavior, and one for the new behavior with strptime?

@zbruick
Copy link
Contributor

zbruick commented Jul 15, 2019

Good catch - yeah that would probably be best.

@deeplycloudy deeplycloudy requested a review from zbruick as a code owner July 21, 2019 16:06
@deeplycloudy
Copy link
Contributor Author

I added two tests: a standalone regex and regex with strptime. I don't understand the flake8 errors on travis, though I gather from above there may be some big-picture things driving those errors, and not this PR?

Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some flake8 issues here. @dopplershift, do you know what the rest of the pydocstyle issues stem from? Any other feedback you have on this?

siphon/catalog.py Outdated Show resolved Hide resolved
siphon/catalog.py Show resolved Hide resolved
siphon/catalog.py Outdated Show resolved Hide resolved
siphon/catalog.py Show resolved Hide resolved
@dopplershift
Copy link
Member

It's been a couple weeks of annoying package incompatibilities. pydocstyle released 4.0, which breaks flake8-docstrings. 😭 I'll push a commit to your branch that should fix it.

flake8-docstrings <= 1.3 and pydocstyle >= 4.0 do not play nice
together. This works around the problem until it's resolved.
@dopplershift
Copy link
Member

Well, that fixed the errors about pydocstyle.

@zbruick
Copy link
Contributor

zbruick commented Aug 5, 2019

Looks like there's just a couple lint issues left to deal with on this and then it's good to go!

@deeplycloudy
Copy link
Contributor Author

deeplycloudy commented Aug 11, 2019

I think the remaining lint issues are handled. Looks like the only remaining Travis problem is a connection error in some part of the code not related to this PR?

@zbruick
Copy link
Contributor

zbruick commented Aug 12, 2019

Thanks for your work on this! So the remaining issue is documented in #273 and doesn't have anything to do with this PR. I think this is good to merge, but I'll defer to @dopplershift.

@dopplershift dopplershift merged commit eea8b0d into Unidata:master Aug 13, 2019
@dopplershift
Copy link
Member

Thanks @deeplycloudy !

@deeplycloudy
Copy link
Contributor Author

My pleasure, @dopplershift and my thanks to you and @zbruick for a pleasant PR process!

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