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.

Sign up for GitHub

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

Jump to bottom

custom error formatter for KeywordArgs #246

Open

mindreframer wants to merge 13 commits into egonSchiele:master from mindreframer:feature/custom_error_formatter

Contributor

mindreframer commented Dec 21, 2016

problem:

it is impossible to quickly understand the contract violation with big hashes

solution:

provide custom error formatter just for keywordargs, that gives you extra information about:
Missing Contract definition (for unspecified keys in contract)
Invalid Args (validation failures)
Missing Args (required values, that are missing)


          custom error formatter for KeywordArgs

9a4fa0c

problem: 
- it is impossible to quickly understand the contract violation with big hashes 

solution: 
- provide custom error formatter just for keywordargs, that gives you extra information about: 
- Missing Contract definition (for unspecified keys in contract)
- Invalid Args (validation failures)
- Missing Args (required values, that are missing)

rous-gg commented Dec 21, 2016

This is a really good feature to have. Right now it's really hard to understand what is wrong with keyword args contract and you spend a lot of time making that diff in mind.

mindreframer added 3 commits

December 21, 2016 12:24


          fix: remove Contracts::Colorize (was used before)

a7a2e16


          fix: dont use new hash args to keep compatibility with Ruby 1.8

3ff3219


          change message building logic for default_error_formatter for consist…

d0d3711

…ency

Contributor Author

mindreframer commented Dec 21, 2016

here is an example how the error message would look like:

ParamContractError:
       Contract violation for argument 1 of 1:
       Expected: (KeywordArgs[{:guid=>String, :occurred_at=>DateTime, :created_by_id=>Contracts::Builtin::Num}])
       Actual: {:guid=>"4fea1b56-3218-4c4c-a2ce-6db42d8e445f", :occurred_at=>"1", :test1=>1}
       Missing Contract: {:test1=>1}
       Invalid Args: [{:occurred_at=>"1", :contract=>DateTime}]
       Missing Args: {:created_by_id=>Contracts::Builtin::Num}
       Value guarded in: Events::AbstractEvent::initialize
       With Contract: KeywordArgs => NilClass

mindreframer added 5 commits

December 21, 2016 13:05


          adjust error formatting just for cucumber specs

c65e92f


          please rubocop

6d839f6


          further rubocop pleasing...

759ff79


          fixes for ruby 1.8

20c0e37


          not gonna please ruby 1.8 anymore, it is ridiculous.

7adfdc4

run error_formatter_spec only for rubies > 1.8

egonSchiele requested changes

View reviewed changes

Owner

egonSchiele left a comment

Very useful, thanks. Nice to see tests too. I added some comments, take a look.

lib/contracts/error_formatter.rb

+                    DefaultErrorFormatter
+                  end
+                  def self.keysword_args?(data)

Owner

egonSchiele Dec 22, 2016

typo, should be keyword_args

lib/contracts/error_formatter.rb

+                  def missing_args_info
+                    @missing_args_info ||= begin
+                      missing_keys = contract_options.keys - arg.keys
+                      contract_options.select do |key, _value|

Owner

egonSchiele Dec 22, 2016

you can use _ instead of _value since that var isn't used

Contributor Author

mindreframer Dec 22, 2016

done

lib/contracts/error_formatter.rb

+                  def missing_contract_info
+                    @missing_contract_info ||= begin
+                      contract_keys = contract_options.keys
+                      arg.select { |key, _value| !contract_keys.include?(key) }

Owner

egonSchiele Dec 22, 2016

same

Contributor Author

mindreframer Dec 22, 2016

done

lib/contracts/error_formatter.rb

+                    else
+                      value.is_a?(contract)
+                    end
+                  rescue

Owner

egonSchiele Dec 22, 2016

why was this rescue needed?

lib/contracts/error_formatter.rb

+                  def check_contract(contract, value)
+                    if contract.respond_to?(:valid?)
+                      contract.valid?(value)

Owner

egonSchiele Dec 22, 2016

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?

rous-gg Dec 22, 2016

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.

rous-gg Dec 22, 2016

Let's take a look at the example:
Invalid Args: [{:occurred_at=>"1", :contract=>DateTime}]
One line of output tells us

which argument is wrong
argument value
contract
This should help developers a lot

Contributor Author

mindreframer Dec 22, 2016

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?

spec/error_formatter_spec.rb

+                end
+                describe "self.class_for" do
+                  it "returns the right formatter for passed in data" do

Owner

egonSchiele Dec 22, 2016

this block needs a test inside it

Contributor Author

mindreframer Dec 22, 2016

done

mindreframer added 4 commits

December 22, 2016 10:34


          fixes typo: keywords_args? -> keyword_args?

4208b23


          replace: _value -> _

fabfe6a


          specs for Contracts::ErrorFormatters.class_for

1630fb5


          for rubocop

04f5fb4

Owner

egonSchiele commented Jan 17, 2017

Hey all,
Sorry for my late response here. @mindreframer could you update the data hash? I like that better than running the contracts twice.

Contributor Author

mindreframer commented Jan 18, 2017

Hey @egonSchiele, good to hear from you!

Because the discussion here ended kinda abruptly and I liked the idea of contracts for Ruby, I have continued my work on a fork here: https://github.com/ddd-ruby/contracts.ruby

Many changes I have introduced would be too disruptive and would be probably rejected here after a longer discussion, and honestly, I would not even blame you for that :). You as maintainer of this project should be more conservative towards changes.

I tried to simplify contracts from the DX perspective, that means

less backwards-compatibility (drop support for rubies 1.8.7- 1.9.3)
faster tests: remove cucumber tests, because they are kinda fragile and expensive to maintain
increase the code climate index (now at 3.9), that required quite some refactoring
reduce cyclomatic complexity as much as possible, that means all the nested if-else blocks were refactored to be much flatter

Overall the resulting codebase should be simpler and a bit easier to maintain / contribute to. One gotcha: the refactoring made it a bit slower, it is roughly 10% slower in trivial benchmarks that the original implementation.

My current fork satisfies my needs and I have already invested enough efforts into it. If you are interested in some of those changes, you might consider merging some parts yourself...

I really hope this does not appear offensive or rude to you. I just did not have the patience to wait for an undefinite time period for a decision from your side. My brain just does not work like this. :P

Overall thanks for Contracts and maybe you will like some parts of my changes and use them.

Best,
Roman

Owner

egonSchiele commented Jan 18, 2017

No problem Roman, it is not offensive at all :) I don't work in Ruby these days, so it is a bit of overhead for me to maintain the project, which is why my response took a while. I'm really glad to see you like the project ... you have made a ton of commits!

I wish I had more time to work on contracts, I would definitely look at your changes. The codebase is quite complex so I would like to simplify it also.

Contributor Author

mindreframer commented Jan 18, 2017

@egonSchiele thx for staying cool! If you don't work in Ruby anymore, maybe you could look around for another maintainer / co-maintainer with rights to merge / push to Rubygems? That way contracts would not suffer a slow death...

A separate Github team would be a nice touch, like: https://github.com/contracts.ruby/contracts
Then you could post a "looking for maintainer(s) for contracts" post on http://www.rubyflow.com/ and link to a github issue with the same title... Some people with more time than you and active Ruby investment might be interested. Pick somebody you would trust to keep the project alive.

Also there are 23 watchers on the repo. )))

But well... That is maybe something you should figure out for yourself :)))

Best,
Roman

Contributor Author

mindreframer commented Jan 18, 2017

here is another project where the original maintainer looked for new maintainers:

fgrehm/vagrant-cachier#143 (comment)

Owner

egonSchiele commented Jan 18, 2017

good idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet