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

Clarifies logic for rescues in Syntax #21

Merged
merged 1 commit into from
Apr 3, 2016

Conversation

lorenrogers
Copy link
Contributor

Rubocop provided warnings about the rescues around the
Date.parse logic. This commit adds documentation around these
functions and makes the rescue logic more specific. return nil
was added to suppress Rubocop warnings and to make the code
more explicit and easier to understand.

Closes #19

Rubocop provided warnings about the rescues around the
`Date.parse` logic. This commit adds documentation around these
functions and makes the rescue logic more specific. `return nil`
was added to suppress Rubocop warnings and to make the code
more explicit and easier to understand.

Closes samwho#19
@samwho
Copy link
Owner

samwho commented Apr 3, 2016

What exactly are the warnings? Why is what you've got here better than what was there before?

@lorenrogers
Copy link
Contributor Author

Here's what a run of Rubocop gives me for the original code:

lib/todo-txt/syntax.rb:42:7: W: Do not suppress exceptions.
      rescue; end
      ^^^^^^^
lib/todo-txt/syntax.rb:49:7: W: Do not suppress exceptions.
      rescue; end
      ^^^^^^^
lib/todo-txt/syntax.rb:56:7: W: Do not suppress exceptions.
      rescue; end
      ^^^^^^^

This commit handles these by having the rescue clause perform an explicit action. I admit that the end result is the same, (Passing nil when the date is invalid,) but the new code/documentation makes it clear that this is the intended behaviour.

Along the way, it also removed the if line =~ CREATED_ON_PATTERN clause, making these three methods consistent and simplifying the logic a little.

@samwho
Copy link
Owner

samwho commented Apr 3, 2016

Neato. This is the first time I've heard of Rubocop. That's an excellent condition for it to pick up on and warn you about.

You also saved us from matching on the same regex twice, which is a nice little performance win. I'll merge this now. :)

@samwho samwho merged commit aed450e into samwho:master Apr 3, 2016
@lorenrogers
Copy link
Contributor Author

Awesome! Yeah, I love Rubocop. Definitely helps keep code clean.

I've used Hound before, which adds in Rubocop checks by default. (More discussion: #16 (comment)) Could be worthwhile for future maintenance.

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.

None yet

2 participants