Refactor of the coercion and validation logic #62

ifellinaholeonce · 2021-04-17T19:00:54Z

Description

Hey there 👋 !
First off, thanks a lot for this gem!

I've written a refactor of the code here in the hopes of making it a little easier to maintain. I know this is a lot of changes coming all at once. I'm wondering if this is something you'd be interested in? Happy to make changes and address any concerns you might have.

Thanks a lot for your time. Looking forward to hearing from you and getting these changes to a place that you'd be interested in merging them in.

Highlights

I've made it a top priority to not change any of the existing specs, to ensure behaviour doesn't change.
I've added unit specs for the classes I've added to maintain high code coverage.
Parameter class was introduced to represent the current param being processed. It takes in name, value, type and options. It knows how to check for defaults, transform and call a new Validator on itself.
Validator class acts as a factory for creating subclasses with validation logic.
Coercion class also acts as a factory for coercing values based on the provided type. The logic for the different types of coercion live in subclasses, ie. Coercion::ArrayParam.

Todos

Additional Notes

Special thanks to @ClaytonPassmore for the help

TODO - Add in validation change from `c80485d05a0431dacdc91d36ac7e196aa04b8548`

Validator unit specs

Unit tests

iMacTia · 2021-04-18T06:44:42Z

Thank you @ifellinaholeonce, this work is impressive! And very much welcome 👏! I want to take the proper time to review this and decide what to do as I think it may deserve a major version shipping.

May I ask what triggered you on doing this refactoring 😮?

ifellinaholeonce · 2021-04-19T00:18:41Z

That's really exciting to hear @iMacTia. Looking forward to hearing your thoughts on it.

Honestly, the refactor started by digging into what I thought was a bug at the time and then it continued on as an opportunity to learn and practice some skills. I had a great time doing it.

iMacTia

@ifellinaholeonce thanks again for the massive contribution 👏!
Overall I'm impressed with the refactoring you've been doing on the gem and I very much like the result!

After careful consideration, I'd be inclined to make this a v1.0 release.
That means we'd not be restricted in keeping the gem fully backwards-compatible, although we want to minimise disruptions in order to make life easier for those upgrading 😄
We'd also need to list all breaking changes into a new UPGRADING.md document we can add to the root of the repo. I'd suggest to add it in this PR and fill it while implementing the braking changes.

If you also like that idea, I've left a few comments that I understand were not an option before this decision, and I hope you'll have the time and will to make these changes.
If that's not the case, then I completely understand that. You've already spent extensive time on this PR, so I'll be happy to take over for you if you prefer to focus your attention elsewhere.

lib/rails_param/param.rb

iMacTia · 2021-04-19T08:54:08Z

lib/rails_param/param.rb


        # validate
-        validate!(params[name], name, options)
+        validate!(parameter)

        if block_given?


We could forward the &block to the parameter initialiser and then rely on the ArrayParameter and HashParameter classes to take care of nested attributes

lib/rails_param/param.rb

spec/spec_helper.rb

ifellinaholeonce · 2021-04-19T22:50:24Z

That's really exciting to hear! 😃 Thanks @iMacTia.

I'm happy to carry on with this. I'll address your comments on this PR. Likely in the next day or two.

* Use ParamEvaluator class for recursion

ifellinaholeonce · 2021-04-30T03:06:12Z

Hey @iMacTia

Just following up here. It took me a little longer than expected, it was a bit of a tough nut to crack but I managed to address your thoughts on the MockController class. Ultimately, I moved all the logic from the base param! method into a new class ParamEvaluator which allowed me to remove the MockController class.

This is the commit where that is introduced: 44d875c

I haven't added a UPGRADING.md since all of these changes so far have been backward compatible.

Looking forward to your thoughts on this approach and where we can go from here.

iMacTia · 2021-04-30T08:45:13Z

Thank you so much @ifellinaholeonce!
I can't wait to review this, will try to do it as soon as possible.
Thanks again for all the hard work 🙏🎉!

iMacTia

This looks great and the important comments were all addressed.
Assuming tests will be green, I feel like we can get this merged, and then it will be just a bit of polishing before we release v1.0 👍

iMacTia · 2021-04-30T09:40:04Z

lib/rails_param/param.rb

-
-  def validate!(param)
-    param.validate
+    ParamEvaluator.new(params).param!(name, type, options, &block)


This is simply beautiful ❤️

nit: this file can now be removed and this method can live in rails_param.rb

iMacTia · 2021-04-30T09:50:25Z

spec/rails_integration_spec.rb

-      expect { get :index, prepare_params(page: ["a", "b", "c"]) }.to raise_error(
-        RailsParam::Param::InvalidParameterError, %q('["a", "b", "c"]' is not a valid Integer))
+      expect { get :index, **prepare_params(page: ["a", "b", "c"]) }.to raise_error(
+        RailsParam::InvalidParameterError, %q('["a", "b", "c"]' is not a valid Integer))


nit: This will be a backwards-incompatible change, as users of this gem will currently be rescuing the old namespace for this exception.
We could either mark this is as a breaking change in the UPGRADING.md file, or being more accommodating and alias the exception with the old namespace 🤔

iMacTia · 2021-04-30T10:28:10Z

lib/rails_param/param_evaluator.rb

+      if block_given?
+        if parameter.type == Array
+          parameter.value.each_with_index do |element, i|
+            if element.is_a?(Hash) || element.is_a?(ActionController::Parameters)
+              recurse element, &block
+            else
+              parameter.value[i] = recurse({ i => element }, i, &block) # supply index as key unless value is hash
+            end
+          end
+        else
+          recurse parameter.value, &block
+        end
+      end


I still feel like this bit should be delegated to the Array and Hash validators, but this can be done on a follow-up PR 👍

iMacTia · 2021-04-30T11:50:16Z

Tests green, I'm merging this in 🎉 !
Thanks again @ifellinaholeonce, this is definitely a monumental piece of work and I really appreciate you taking the time to help us out.

Actually I was wondering if you'd be considering becoming a maintainer of this gem?
Think about it and let me know, we could then ask @nicolasblanco if he agrees as ultimately he'll need to make you one 😄

ifellinaholeonce · 2021-05-05T14:00:57Z

Hey @iMacTia I'm interested in learning a bit more about becoming a maintainer for the gem😄

Would you be open to a bit of back and forth about it? Happy to do email, or open an issue in this repo or anything really if you have a preference.

iMacTia · 2021-05-09T08:14:49Z

Absolutely! We can start having a quick chat on the next steps and in the meantime I'll try reaching out to @nicolasblanco.
Are you on Gitter? If you are in any public room I can find you there and we can have a private chat.
Open to other solutions as well if you have idea, just trying to find a way to share contact details without letting the whole internet know 😂

ifellinaholeonce · 2021-05-10T02:10:50Z

haha yes I struggled with that same problem. You can find me on Gitter in the rails community 😄

ifellinaholeonce added 30 commits December 5, 2020 11:51

Bump ruby version

620dc95

add seams

d41efcf

Add objects for different param types

eb4562b

Coercion class

d99b891

raise TypeError

e7a8222

Remove unneeded constant

45fdf2c

Move hash and array check inside klass_for

dbe5572

Add param module namespace

ad8b3b0

Move out error

94b4104

add parameter class

d7b499c

move validate to parameter class

ed7c9a2

add class namespace

4f90acb

add validator

6800efc

is validator

116fd01

add attr reader values to validator

51cce84

add min validator

c79c116

add max validator

badf419

min_length validator

81c60a1

add max_length validator

99d729d

add custom validator

735650a

remove validator case statement

2833f7b

add tmp to gitignore

b021cb0

param type mapping for coercion

ffc141c

clean up

c8a77f2

boolean constants

0ee3646

clean up time param

74e9b0c

coerce value before initalizing parameter

5fe2dac

delegate parameter values

51d5b67

validatable options

d9c02c9

blank validator clean up

85c3bb3

ifellinaholeonce and others added 12 commits April 3, 2021 11:26

max length spec

61674f8

max, min, min_length specs

6d94b47

Merge branch 'refactor'

b4e5314

TODO - Add in validation change from `c80485d05a0431dacdc91d36ac7e196aa04b8548`

Merge remote-tracking branch 'origin/master' into specs/new

bfa58da

Merge branch 'specs/new' into specs/new_validator_specs

0cbe325

Merge remote-tracking branch 'origin/master' into specs/new

7bca219

Merge branch 'specs/new' into specs/new_validator_specs

0458513

Update invalidparam error raise

d2264c0

Merge pull request #5 from ifellinaholeonce/specs/new_validator_specs

fcb7733

Validator unit specs

Merge pull request #4 from ifellinaholeonce/specs/new

dcd9f6e

Unit tests

handle ActionController:Parameters as Hash

a932a64

return nil from coercion if param is nil

7292487

iMacTia requested changes Apr 19, 2021

View reviewed changes

ifellinaholeonce added 3 commits April 21, 2021 22:33

remove param module

30b1539

clean up spec helper

ef7338d

Refactor recursion (#6)

44d875c

* Use ParamEvaluator class for recursion

ifellinaholeonce requested a review from iMacTia April 30, 2021 03:06

Remove empty file

d5fd94a

iMacTia approved these changes Apr 30, 2021

View reviewed changes

iMacTia merged commit 7ef3018 into nicolasblanco:master Apr 30, 2021

iMacTia mentioned this pull request May 15, 2021

Move recursive logic into Validators #66

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of the coercion and validation logic #62

Refactor of the coercion and validation logic #62

ifellinaholeonce commented Apr 17, 2021

iMacTia commented Apr 18, 2021

ifellinaholeonce commented Apr 19, 2021

iMacTia left a comment

iMacTia Apr 19, 2021

ifellinaholeonce commented Apr 19, 2021

ifellinaholeonce commented Apr 30, 2021

iMacTia commented Apr 30, 2021

iMacTia left a comment

iMacTia Apr 30, 2021

iMacTia Apr 30, 2021

iMacTia Apr 30, 2021

iMacTia Apr 30, 2021

iMacTia commented Apr 30, 2021

ifellinaholeonce commented May 5, 2021

iMacTia commented May 9, 2021

ifellinaholeonce commented May 10, 2021

Refactor of the coercion and validation logic #62

Refactor of the coercion and validation logic #62

Conversation

ifellinaholeonce commented Apr 17, 2021

Description

Todos

Additional Notes

iMacTia commented Apr 18, 2021

ifellinaholeonce commented Apr 19, 2021

iMacTia left a comment

Choose a reason for hiding this comment

iMacTia Apr 19, 2021

Choose a reason for hiding this comment

ifellinaholeonce commented Apr 19, 2021

ifellinaholeonce commented Apr 30, 2021

iMacTia commented Apr 30, 2021

iMacTia left a comment

Choose a reason for hiding this comment

iMacTia Apr 30, 2021

Choose a reason for hiding this comment

iMacTia Apr 30, 2021

Choose a reason for hiding this comment

iMacTia Apr 30, 2021

Choose a reason for hiding this comment

iMacTia Apr 30, 2021

Choose a reason for hiding this comment

iMacTia commented Apr 30, 2021

ifellinaholeonce commented May 5, 2021

iMacTia commented May 9, 2021

ifellinaholeonce commented May 10, 2021