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

Add capability to deal with stations with no identifier from Wyoming #239

Merged
merged 1 commit into from
Jul 18, 2018

Conversation

jrleeman
Copy link
Contributor

Closes #238

Deals with missing lines from the wyoming archive when the station has a number, but no identifier.


# If there are only 26 lines, the station doesn't have a name identified
# and we need to insert a record showing this for parsing to proceed.
if len(lines) == 26:
Copy link
Member

Choose a reason for hiding this comment

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

You couldn't instead check:

lines[1].startswith('Station identifier')

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could sure

Copy link
Member

Choose a reason for hiding this comment

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

Just seems like it could change from 26 lines for a variety of reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but we'll break on all of them 😭
Push coming.

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Current check just seems pretty brittle...would be good to check for the exact condition that's missing.

@jrleeman
Copy link
Contributor Author

Happy to change it, though I don't really see it being more or less brittle as any other weirdness in the footer will still break things. I'll revise now.

@jrleeman jrleeman force-pushed the wyoming_metadata_parse branch from dd99627 to d63a2d9 Compare July 17, 2018 20:04
@jrleeman
Copy link
Contributor Author

Ok, not quite as easy as your startswith because we have a lot of leading whitespace and the field is totally missing, not just unpopulated. Anywho, this should do the trick.

@dopplershift
Copy link
Member

Right, if it's missing then (now accounting for whitespace) lines[1].strip().startswith('Station identifier:') should return False, no? Your new way is fine, I just wasn't sure if there was something else I'm missing.

@jrleeman
Copy link
Contributor Author

The whitespace... It would have to have 28 leading whitespaces on the string. Ugly.

@dopplershift
Copy link
Member

Is there a reason the strip() call wouldn’t remove the need to hard-code the spaces?

@jrleeman
Copy link
Contributor Author

Probably not. Is there any reason that .strip().startswith('...') is better/worse than if 'foo' in bar: ?

@dopplershift
Copy link
Member

That’s fine. But my original suggestion was to look for “Station Identifier” to be missing from lines[1], yours is to look only for “Station number” there. I just was confused as to why my approach, which is checking directly for the case that you’re solving, wouldn’t work.

Either way is fine, and I don’t want to bike she’d this to death. Just trying to see if I’m missing something.

@jrleeman
Copy link
Contributor Author

I just think this is a bit clearer and they both accomplish the same task with the same complexity as far as I can tell. Tomato vs Tomato?

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

Successfully merging this pull request may close these issues.

2 participants