enhanced the error message of pick function. #179
Conversation
| @@ -29,6 +29,6 @@ | |||
| end | |||
|
|
|||
| it 'should error if no values are passed' do | |||
| expect { scope.function_pick([]) }.to raise_error(Puppet::Error, /Must provide non empty value./) | |||
There was a problem hiding this comment.
Using expect { }.to is preferred over lambda { }.should. In addition it's best practice to do a string match against the error message to ensure that we're catching the right error, instead of any error of the right type.
There was a problem hiding this comment.
Thanks for explaining! I will change that.
|
Thank you very much for this contribution! I've added some inline comments on this pull request about some stylistic concerns on this pull request, but when those addressed we should be able to get this merged in. |
Fix the spec test to use expect {}.to instead of lambda {}.should
as explained by Adrienthebo.
"Using expect { }.to is preferred over lambda { }.should.
In addition it's best practice to do a string match against the
error message to ensure that we're catching the right error,
instead of any error of the right type."
Also fixed a typo on the error message, it was missing one space.
|
Sorry, one final thing - would you be willing to rebase the two commits in this pull request into a single commit? |
| @@ -20,10 +20,8 @@ module Puppet::Parser::Functions | |||
| args.delete(:undef) | |||
| args.delete(:undefined) | |||
| args.delete("") | |||
| if args[0].to_s.empty? then | |||
There was a problem hiding this comment.
HI Adrien,
I will revert to the previous layout. Will only change the message and the call to fail to raise the correct exception.
Puppet::ParseError
Is that correct ?
There was a problem hiding this comment.
That would be perfect, yes.
|
after making that changes i will rebase into a single commit! :-) I am trying to improve my ruby skills and decided to contribute to projects I use, as a way to do that.! |
|
sorry, did not had time to look over this. on monday i will finish it! I had removed one wisdom tooth this week. :-/ |
When pick function fail return a better error message like the other stdlib functions, indicating that the error is on function pick. This would help people that see the error to identity it is related to a incorrect use of stdlib function pick, instead of having to grep all puppet libraries and manifests source for the old message. I had also changed the spec test. pick function change spec as suggested puppetlabsGH-179 Fix the spec test to use expect {}.to instead of lambda {}.should as explained by Adrienthebo. "Using expect { }.to is preferred over lambda { }.should. In addition it's best practice to do a string match against the error message to ensure that we're catching the right error, instead of any error of the right type." Also fixed a typo on the error message, it was missing one space. pick function stylish fix as suggested on GH179
|
@adrienthebo Please merge. :-) |
enhanced the error message of pick function.
|
summary: merged into master in ebec9de; thanks for the contribution! |
When pick function fail return a better error message like
the other stdlib functions, indicating that the error
is on function pick.
This would help people that see the error to identity it is
related to a incorrect use of stdlib function pick, instead of having
to grep all puppet libraries and manifests source for the old message.
I had also changed the spec test.