pat / combustion

Simple, elegant testing for Rails Engines
MIT License
709 stars 51 forks source link

use keyboard arguments instead of positional #110

Closed bishwahang closed 4 years ago

bishwahang commented 4 years ago

Currently it throws warning when positional arguments are used:

/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/combustion-1.1.2/lib/combustion/database/reset.rb:22: Passing permitted_classes with the 2nd argument of Psych.safe_load is deprecated. Use keyword argument like Psych.safe_load(yaml, permitted_classes: ...) instead.
/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/combustion-1.1.2/lib/combustion/database/reset.rb:22: Passing permitted_symbols with the 3rd argument of Psych.safe_load is deprecated. Use keyword argument like Psych.safe_load(yaml, permitted_symbols: ...) instead.
/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/combustion-1.1.2/lib/combustion/database/reset.rb:22: Passing aliases with the 4th argument of Psych.safe_load is deprecated. Use keyword argument like Psych.safe_load(yaml, aliases: ...) instead.

If we look at Pysch gem, they propose using keyword arguments instead.

pat commented 4 years ago

Thanks for this PR :)

Is it possible to re-add 2.3 into the test matrix and remove the minimum Ruby requirement? I'd like to offer extensive backwards compatibility if possible - and while I'm generally in favour of keeping gems relatively up-to-date with modern Ruby, I feel a testing-related gem like Combustion should be more relaxed about what it can work with.

bishwahang commented 4 years ago

Hi @pat , rubocop doesn’t support 2.3 and travis checks were failing because of that. Shall it still be included and ignore the failed check?

pat commented 4 years ago

I realise you're maybe working through some fixes now :) but also: the Rubocop issue is my fault, I'll try and get it sorted tonight. 🤞🏻

bishwahang commented 4 years ago

@pat Looks like the rubocop issue got solved with the latest PR merge https://github.com/pat/combustion/pull/111/files#diff-8b7db4d5cc4b8f6dc8feb7030baa2478R15-R18

Once the checks pass, this PR should be ready for review.

pat commented 4 years ago

All looking good! Thanks for this :)

pat commented 4 years ago

Just published 1.3.1 which includes this PR - thank you again!

bishwahang commented 4 years ago

@pat Awesome! Thank you too, for the the gem. :)