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

Raise a better error when no String is given in #html_escape #840

Merged
merged 3 commits into from Mar 23, 2016

Conversation

Projects
None yet
2 participants
@rosetree
Copy link
Contributor

commented Mar 22, 2016

Nanoc::Helpers::HTMLEscape#html_escape breaks when the given parameter
is not a String.

The following message appeared:

Captain! We’ve been hit!

Message:
NoMethodError: undefined method `gsub' for 1:Fixnum

Stack trace:
0. […]/nanoc-4.1.4/lib/nanoc/helpers/html_escape.rb:38:in `html_escape'
1. […]/nanoc-4.1.4/lib/nanoc/helpers/link_to.rb:59:in `block in link_to'
2. […]/nanoc-4.1.4/lib/nanoc/helpers/link_to.rb:58:in `each'
3. […]/nanoc-4.1.4/lib/nanoc/helpers/link_to.rb:58:in `reduce'
4. […]/nanoc-4.1.4/lib/nanoc/helpers/link_to.rb:58:in `link_to'
5. layout /default/:84:in `block in render'

After I tried something similar to the following:

link_to 'Homepage', @items['/'].path, {accesskey: 1}

To fix this issue I had multiple ideas:

I first wanted to use value.to_s in the Nanoc Helper link_to
(currently on line 59). But it is not garanteed, that to_s results in
a useful output.

https://github.com/nanoc/nanoc/blob/master/lib/nanoc/helpers/link_to.rb#L59

My second thought was to use string.to_s in #html_escape, unless string.is_a? String. But again, it is not clear, if the result is what
the user expected.

So I came up with my third idea, to raise a RuntimeError with slightly
more helpful error message. This commit adds an error message, that
points the user to the origin of his error. Using the stack trace, it
should not be to hard to investigate the source of your mistake.


I based this from branch master, because the commit doesn’t really
fix an error, it just enhances the error message.

Raise a better error when no String is given in #html_escape
Nanoc::Helpers::HTMLEscape#html_escape breaks when the given parameter
is not a String.

The following message appeared:

~~~~~
Captain! We’ve been hit!

Message:
NoMethodError: undefined method `gsub' for 1:Fixnum

Stack trace:
0. […]/nanoc-4.1.4/lib/nanoc/helpers/html_escape.rb:38:in `html_escape'
1. […]/nanoc-4.1.4/lib/nanoc/helpers/link_to.rb:59:in `block in link_to'
2. […]/nanoc-4.1.4/lib/nanoc/helpers/link_to.rb:58:in `each'
3. […]/nanoc-4.1.4/lib/nanoc/helpers/link_to.rb:58:in `reduce'
4. […]/nanoc-4.1.4/lib/nanoc/helpers/link_to.rb:58:in `link_to'
5. layout /default/:84:in `block in render'
~~~~~

After I tried something similar to the following:

~~~~~ruby
link_to 'Homepage', @Items['/'].path, {accesskey: 1}
~~~~~~

To fix this issue I had multiple ideas:

I first wanted to use `value.to_s` in the Nanoc Helper link_to
(currently on line 59). But it is not garanteed, that `to_s` results in
a useful output.

https://github.com/nanoc/nanoc/blob/master/lib/nanoc/helpers/link_to.rb#L59

My second thought was to use `string.to_s` in #html_escape, `unless
string.is_a? String`. But again, it is not clear, if the result is what
the user expected.

So I came up with my third idea, to raise a RuntimeError with slightly
more helpful error message. This commit adds an error message, that
points the user to the origin of his error. Using the stack trace, it
should not be to hard to investigate the source of your mistake.
@ddfreyne

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

Looks interesting! I’m typically not fond of doing type checks, but I don’t see an alternative in this case.

Two remarks:

  • Prefer raising an ArgumentError in this case
  • Can you add a test for this?

rosetree added some commits Mar 23, 2016

@rosetree

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

Prefer raising an ArgumentError in this case

You’re right.

Can you add a test for this?

Yes; done.


As you, I don’t think type checks are the most elegant solutions, but I didn’t see a better way here. An ArgumentError is imho better, compared to a NoMethodError.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

Cool, thanks!

(I think an even better way would be to use #to_str; see this gist for details. But that’s really getting into the details, so I’m OK with the implementation as it is right now.)

@ddfreyne ddfreyne merged commit 6958301 into nanoc:master Mar 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rosetree rosetree deleted the rosetree:feature/use-better-error-in-html-escape branch Mar 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.