-
Notifications
You must be signed in to change notification settings - Fork 82
custom error formatter for KeywordArgs #246
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Open
mindreframer
wants to merge
13
commits into
egonSchiele:master
Choose a base branch
from
mindreframer:feature/custom_error_formatter
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
9a4fa0c
custom error formatter for KeywordArgs
mindreframer a7a2e16
fix: remove Contracts::Colorize (was used before)
mindreframer 3ff3219
fix: dont use new hash args to keep compatibility with Ruby 1.8
mindreframer d0d3711
change message building logic for default_error_formatter for consist…
mindreframer c65e92f
adjust error formatting just for cucumber specs
mindreframer 6d839f6
please rubocop
mindreframer 759ff79
further rubocop pleasing...
mindreframer 20c0e37
fixes for ruby 1.8
mindreframer 7adfdc4
not gonna please ruby 1.8 anymore, it is ridiculous.
mindreframer 4208b23
fixes typo: keywords_args? -> keyword_args?
mindreframer fabfe6a
replace: _value -> _
mindreframer 1630fb5
specs for Contracts::ErrorFormatters.class_for
mindreframer 04f5fb4
for rubocop
mindreframer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
module Contracts | ||
class ErrorFormatters | ||
def self.failure_msg(data) | ||
class_for(data).new(data).message | ||
end | ||
|
||
def self.class_for(data) | ||
return Contracts::KeywordArgsErrorFormatter if keyword_args?(data) | ||
DefaultErrorFormatter | ||
end | ||
|
||
def self.keyword_args?(data) | ||
data[:contract].is_a?(Contracts::Builtin::KeywordArgs) && data[:arg].is_a?(Hash) | ||
end | ||
end | ||
|
||
class DefaultErrorFormatter | ||
attr_accessor :data | ||
def initialize(data) | ||
@data = data | ||
end | ||
|
||
def message | ||
%{#{header} | ||
Expected: #{expected}, | ||
Actual: #{data[:arg].inspect} | ||
Value guarded in: #{data[:class]}::#{method_name} | ||
With Contract: #{data[:contracts]} | ||
At: #{position} } | ||
end | ||
|
||
private | ||
|
||
def header | ||
if data[:return_value] | ||
"Contract violation for return value:" | ||
else | ||
"Contract violation for argument #{data[:arg_pos]} of #{data[:total_args]}:" | ||
end | ||
end | ||
|
||
def expected | ||
Contracts::Formatters::Expected.new(data[:contract]).contract | ||
end | ||
|
||
def position | ||
Contracts::Support.method_position(data[:method]) | ||
end | ||
|
||
def method_name | ||
Contracts::Support.method_name(data[:method]) | ||
end | ||
end | ||
|
||
class KeywordArgsErrorFormatter < DefaultErrorFormatter | ||
def message | ||
s = [] | ||
s << "#{header}" | ||
s << " Expected: #{expected}" | ||
s << " Actual: #{data[:arg].inspect}" | ||
s << " Missing Contract: #{missing_contract_info}" unless missing_contract_info.empty? | ||
s << " Invalid Args: #{invalid_args_info}" unless invalid_args_info.empty? | ||
s << " Missing Args: #{missing_args_info}" unless missing_args_info.empty? | ||
s << " Value guarded in: #{data[:class]}::#{method_name}" | ||
s << " With Contract: #{data[:contracts]}" | ||
s << " At: #{position} " | ||
|
||
s.join("\n") | ||
end | ||
|
||
private | ||
|
||
def missing_args_info | ||
@missing_args_info ||= begin | ||
missing_keys = contract_options.keys - arg.keys | ||
contract_options.select do |key, _| | ||
missing_keys.include?(key) | ||
end | ||
end | ||
end | ||
|
||
def missing_contract_info | ||
@missing_contract_info ||= begin | ||
contract_keys = contract_options.keys | ||
arg.select { |key, _| !contract_keys.include?(key) } | ||
end | ||
end | ||
|
||
def invalid_args_info | ||
@invalid_args_info ||= begin | ||
invalid_keys = [] | ||
arg.each do |key, value| | ||
contract = contract_options[key] | ||
next unless contract | ||
invalid_keys.push(key) unless check_contract(contract, value) | ||
end | ||
invalid_keys.map do |key| | ||
{key => arg[key], :contract => contract_options[key] } | ||
end | ||
end | ||
end | ||
|
||
def check_contract(contract, value) | ||
if contract.respond_to?(:valid?) | ||
contract.valid?(value) | ||
else | ||
value.is_a?(contract) | ||
end | ||
rescue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why was this rescue needed? |
||
false | ||
end | ||
|
||
def contract_options | ||
@contract_options ||= data[:contract].send(:options) | ||
end | ||
|
||
def arg | ||
data[:arg] | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
RSpec.describe "Contracts::ErrorFormatters" do | ||
before :all do | ||
@o = GenericExample.new | ||
end | ||
C = Contracts::Builtin | ||
|
||
describe "self.class_for" do | ||
let(:keywordargs_contract) { C::KeywordArgs[:name => String, :age => Fixnum] } | ||
let(:other_contract) { [C::Num, C::Num, C::Num] } | ||
|
||
it "returns KeywordArgsErrorFormatter for KeywordArgs contract" do | ||
data_keywordargs = {:contract => keywordargs_contract, :arg => {:b => 2}} | ||
expect(Contracts::ErrorFormatters.class_for(data_keywordargs)).to eq(Contracts::KeywordArgsErrorFormatter) | ||
end | ||
|
||
it "returns Contracts::DefaultErrorFormatter for other contracts" do | ||
data_default = {:contract => other_contract, :arg => {:b => 2}} | ||
expect(Contracts::ErrorFormatters.class_for(data_default)).to eq(Contracts::DefaultErrorFormatter) | ||
end | ||
end | ||
|
||
def format_message(str) | ||
str.split("\n").map(&:strip).join("\n") | ||
end | ||
|
||
def fails(msg, &block) | ||
expect { block.call }.to raise_error do |e| | ||
expect(e).to be_a(ParamContractError) | ||
expect(format_message(e.message)).to include(format_message(msg)) | ||
end | ||
end | ||
|
||
if ruby_version > 1.8 | ||
describe "self.failure_msg" do | ||
it "includes normal information" do | ||
msg = %{Contract violation for argument 1 of 1: | ||
Expected: (KeywordArgs[{:name=>String, :age=>Fixnum}]) | ||
Actual: {:age=>"2", :invalid_third=>1} | ||
Missing Contract: {:invalid_third=>1} | ||
Invalid Args: [{:age=>"2", :contract=>Fixnum}] | ||
Missing Args: {:name=>String} | ||
Value guarded in: GenericExample::simple_keywordargs | ||
With Contract: KeywordArgs => NilClass} | ||
fails msg do | ||
@o.simple_keywordargs(:age => "2", :invalid_third => 1) | ||
end | ||
end | ||
|
||
it "includes Missing Contract information" do | ||
fails %{Missing Contract: {:invalid_third=>1, :invalid_fourth=>1}} do | ||
@o.simple_keywordargs(:age => "2", :invalid_third => 1, :invalid_fourth => 1) | ||
end | ||
end | ||
|
||
it "includes Invalid Args information" do | ||
fails %{Invalid Args: [{:age=>"2", :contract=>Fixnum}]} do | ||
@o.simple_keywordargs(:age => "2", :invalid_third => 1) | ||
end | ||
end | ||
|
||
it "includes Missing Args information" do | ||
fails %{Missing Args: {:name=>String}} do | ||
@o.simple_keywordargs(:age => "2", :invalid_third => 1) | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of calling the
valid
method of a contract again, to display the error message, for a couple of reasons:It breaks the idea that contracts are only run once...so if someone happens to write a contract that takes a long time to check, they wouldn't expect the error message to take time to render.
Error rendering should ideally be very simple. It shouldn't be possible to generate an error while rendering the error message. But code like this means we'll be running arbitrary code by other folks (anyone can write their own class with a valid? functions), which could fail for whatever reason. Maybe that is why you added the
rescue
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now data object does not contain information about specific invalid keyword args keys. it's really hard to make a diff in mind to find them. in most situations contracts are really simple one (type checks, range checks etc) and they do not take much time to execute. Actually we can move that to config so developers will be able to enable/disable this failure msg calculation globally. The main purpose for it is to save time when working in test and dev modes. What do you think if we move that to config?
We create a lot of custom contracts in our applications utilising :valid? method. You can both return false in it or raise ParamContractError. This is why we've added rescue to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take a look at the example:
Invalid Args: [{:occurred_at=>"1", :contract=>DateTime}]
One line of output tells us
This should help developers a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you have expensive logic in your contract validators, you are doing it wrong. Elixir for example allows only some selected bits of logic to be run in function guards, to make sure that your function call is not burdened by accidental side-effects from guards. Sure, here it is theoretically possible to do anything, but that responsibility should be up to the user of the library. On the other hand, it would be possible to modify the
data
hash to include all the information about failed/missing validations, then we would not need to run validations twice for error message generation... Life is about tradeoffs... @egonSchiele Which alternative would you prefer?