Skip to content

Sjw/timestamp validation#12

Open
supernullset wants to merge 5 commits intomasterfrom
sjw/timestamp-validation
Open

Sjw/timestamp validation#12
supernullset wants to merge 5 commits intomasterfrom
sjw/timestamp-validation

Conversation

@supernullset
Copy link

Adds validation to _t values, ensuring that the values are valid unix timestamps.

lib/kmts.rb Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you factor this validation logic into a separate method? Raising the exception only to be caught locally seems a bit ungainly, though I understand it's necessary given #log_error.

Otherwise, note your regular expression pattern is unanchored and arguably a bit overspecified; I'd suggest /\A\d+\z/ instead. It's also necessary to return after handling the exception, as otherwise the tracking request is still issued. Finally as a very minor style nit, our guide advises against and in favor of &&.

Copy link
Author

Choose a reason for hiding this comment

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

I could! I thought that since its only going to be used right here, bringing it out would be overkill.

Totally agree about the raise here, I am not happy about it but I am working with the patterns in here.

Good calls on the rest, thanks for spotting it!

@supernullset
Copy link
Author

@njm do we want to return here as well? I think we would.

@njm
Copy link
Contributor

njm commented Apr 1, 2015

Couldn't hurt as a defensive play, though at the same time I'd be comfortable leaving that piece untouched.

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.

2 participants