Update handling of argument errors #97

afuno · 2022-10-23T20:54:01Z

This PR updates the argument error handling.

All errors are put into an array one by one.

Within this PR, only the first error is raised. This preserves the underlying logic when working with services.

The purpose of this PR is to solve the problem of ignoring the first checks in favor of the last. An example is presented here: #95

lib/service_actor/checkable.rb

sunny · 2022-10-24T07:54:04Z

Neat, this looks like a sound approach! 👍🏻

Ideally:

each check class could be able to tell by itself if it needs to be applied or not depending on the input key (and other options), rather than having the condition inside the general Checkable
we can reduce the duplication of all the conditions down to an array of available check classes
we can re-use the same logic on inputs and outputs

afuno · 2022-11-05T23:04:57Z

@sunny I have added the changes. Please take a look when you have time. The changes mainly concern the Checkable class, where I have improved the look of the code, also considering your recommendations.

lib/service_actor/checkable.rb

lib/service_actor/checks/default_check.rb

lib/service_actor/core.rb

afuno · 2022-11-06T21:38:11Z

@sunny I propose to update the comments on the code and edits :)

Buzzkill-McSquare

I mostly left comments pertaining to defending against missing keys when gathering values from advanced inputs. Mostly around :message, but we might want to validate the presence of both it and :is as well.

Buzzkill-McSquare · 2022-11-06T22:38:31Z

lib/service_actor/checks/default_check.rb

+      return add_argument_error(
+        message,
+        input_key: @input_key,
+        actor: self.class,
+      )


What happens if someone sets the value of default: {}? Should we have a fallback?

Suggested change

return add_argument_error(

message,

input_key: @input_key,

actor: self.class,

)

if message.nil?

return "The \"#{@input_key}\" input on \"#{@actor}\" is missing"

else

return add_argument_error(

message,

input_key: @input_key,

actor: self.class,

)

end

Buzzkill-McSquare · 2022-11-06T22:43:09Z

lib/service_actor/checks/inclusion_check.rb

+
+  def define_inclusion_and_message
+    if @inclusion.is_a?(Hash)
+      @inclusion.values_at(:in, :message)


Like ServiceActor::Checks::DefaultCheck, what should we do in the vent that someone doesn't include the :message key in the advanced mode hash?

Buzzkill-McSquare · 2022-11-06T22:47:34Z

lib/service_actor/checks/must_check.rb

+
+  def define_check_and_message_from(nested_check_conditions)
+    if nested_check_conditions.is_a?(Hash)
+      nested_check_conditions.values_at(:is, :message)


Just pointing out that we might want to handle situations where the :message key isn't present, as mentioned in other comments.

Buzzkill-McSquare · 2022-11-06T22:49:52Z

lib/service_actor/checks/nil_check.rb

+  end
+
+  def check
+    return unless @value.nil?


Should we be making this check? What if the argument is allow_nil: false?

In this place everything should be correct. If false is passed, then the input will expect a value.

Buzzkill-McSquare · 2022-11-06T22:50:23Z

lib/service_actor/checks/nil_check.rb

+
+  def define_allow_nil_and_message_from(allow_nil)
+    if allow_nil.is_a?(Hash)
+      allow_nil.values_at(:is, :message)


One (second to) last comment to point out that we might want to defend against missing :message keys.

Buzzkill-McSquare · 2022-11-06T22:54:17Z

lib/service_actor/checks/type_check.rb

+      input_key: input_key,
+      actor: actor,
+      type_definition: conditions,
+      given_type: result[input_key],


Should we give the actual class object instead of the value?

Suggested change

given_type: result[input_key],

given_type: result[input_key].class,

It breaks behavior. This is not the purpose of this PR.

Buzzkill-McSquare · 2022-11-06T22:54:53Z

lib/service_actor/checks/type_check.rb

+  def define_types_and_message
+    if @type_definition.is_a?(Hash)
+      @type_definition, message =
+        @type_definition.values_at(:is, :message)


Last callout about defending against a missing :message key.

lib/service_actor/core.rb

afuno · 2022-11-06T23:22:43Z

@Buzzkill-McSquare Thanks for the comments. These areas really need to be improved. I will try to improve it, but as part of a separate PR. In this PR, I'm trying to replace a slightly different functionality.

Buzzkill-McSquare · 2022-11-07T02:25:21Z

Got it. I wasn't entirely sure of the scope of the changes here, sorry for suggesting some creep.

afuno · 2022-11-26T11:08:42Z

@sunny Look, please, once again at the code.

sunny

Awesome work. Sounds like we can continue in this direction 👍🏻

Good points raised by @Buzzkill-McSquare as well about the empty message arguments, but that should probably be another PR on top of this once this is merged.

lib/service_actor/checkable.rb

sunny · 2022-12-20T09:40:58Z

Hi Anton! In regard to instance.send(:raise_accumulated_errors I have pushed a few suggestions on your branch. I think moving raise_accumulated_errors out of the core this makes the code simpler with less dependencies between the core and the other parts. Also, raising errors for inputs and outputs are kept together.

Could you have a look at what I committed and let me know what you think?

afuno · 2022-12-20T09:45:27Z

Hi Anton! In regard to instance.send(:raise_accumulated_errors I have pushed a few suggestions on your branch. I think moving raise_accumulated_errors out of the core this makes the code simpler with less dependencies between the core and the other parts. Also, raising errors for inputs and outputs are kept together.

Could you have a look at what I committed and let me know what you think?

Hello! I like this. It's much easier and better with this. Thanks 🙂

sunny · 2022-12-29T18:00:46Z

This has been released under v3.6.0 🎉

Thanks a lot for your contribution @afuno, this is a much better way of handling errors!

Happy holidays ✨

afuno · 2022-12-29T22:21:27Z

@sunny Thanks. Happy Holidays to you too 🎆

afuno added 4 commits October 23, 2022 23:45

New handling of arguments errors

ea76efa

Deleted modules of the old implementation of checks

1c60ef0

Raisable renamed to Configurable

bf5a744

Deleted code after testing

d210437

afuno mentioned this pull request Oct 23, 2022

Problems between checks within the same input #95

Closed

afuno commented Oct 23, 2022

View reviewed changes

lib/service_actor/checkable.rb Outdated Show resolved Hide resolved

afuno added 8 commits November 5, 2022 17:21

Merge branch 'main' into feature/collection_of_argument_errors

a848f79

Conditions moved to class

ea6de12

Refactoring

e751885

Refactoring

3e733f6

Deleted unused code

5009e2a

Refactoring

071ebcd

Deleted unused code

a4ba06f

checker —> check

ff66fce

afuno added 2 commits November 6, 2022 04:19

Improvement

5be8e1b

.for —> .check

77434c1

sunny reviewed Nov 6, 2022

View reviewed changes

afuno added 3 commits November 7, 2022 00:19

Refactoring

419fe38

argument_errors —> service_actor_argument_errors

cd67f44

Refactoring

3ac2410

Merge branch 'main' into feature/collection_of_argument_errors

09cda79

Buzzkill-McSquare reviewed Nov 6, 2022

View reviewed changes

sunny reviewed Dec 4, 2022

View reviewed changes

lib/service_actor/checkable.rb Outdated Show resolved Hide resolved

lib/service_actor/checkable.rb Outdated Show resolved Hide resolved

Refactoring

0a09d2d

afuno added 2 commits December 5, 2022 10:38

Deleted comment

95083c7

Refactoring

4743214

sunny reviewed Dec 5, 2022

View reviewed changes

lib/service_actor/checkable.rb Outdated Show resolved Hide resolved

sunny mentioned this pull request Dec 5, 2022

Raise an error if the :message option is missing on checks #100

Closed

afuno marked this pull request as ready for review December 7, 2022 15:48

Move error checking out of core

3148f46

sunny force-pushed the feature/collection_of_argument_errors branch from 7aaed33 to 3148f46 Compare December 20, 2022 09:34

sunny approved these changes Dec 20, 2022

View reviewed changes

Merge branch 'main' into feature/collection_of_argument_errors

2f12b43

sunny merged commit 0c90316 into sunny:main Dec 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update handling of argument errors #97

Update handling of argument errors #97

afuno commented Oct 23, 2022 •

edited

Loading

sunny commented Oct 24, 2022

afuno commented Nov 5, 2022

afuno commented Nov 6, 2022

Buzzkill-McSquare left a comment

Buzzkill-McSquare Nov 6, 2022

Buzzkill-McSquare Nov 6, 2022

Buzzkill-McSquare Nov 6, 2022

Buzzkill-McSquare Nov 6, 2022

afuno Nov 6, 2022

Buzzkill-McSquare Nov 6, 2022

Buzzkill-McSquare Nov 6, 2022

afuno Nov 6, 2022

Buzzkill-McSquare Nov 6, 2022

afuno commented Nov 6, 2022

Buzzkill-McSquare commented Nov 7, 2022

afuno commented Nov 26, 2022

sunny left a comment

sunny commented Dec 20, 2022

afuno commented Dec 20, 2022

sunny commented Dec 29, 2022 •

edited

Loading

afuno commented Dec 29, 2022

	given_type: result[input_key],
	given_type: result[input_key].class,

Update handling of argument errors #97

Update handling of argument errors #97

Conversation

afuno commented Oct 23, 2022 • edited Loading

sunny commented Oct 24, 2022

afuno commented Nov 5, 2022

afuno commented Nov 6, 2022

Buzzkill-McSquare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afuno commented Nov 6, 2022

Buzzkill-McSquare commented Nov 7, 2022

afuno commented Nov 26, 2022

sunny left a comment

Choose a reason for hiding this comment

sunny commented Dec 20, 2022

afuno commented Dec 20, 2022

sunny commented Dec 29, 2022 • edited Loading

afuno commented Dec 29, 2022

afuno commented Oct 23, 2022 •

edited

Loading

sunny commented Dec 29, 2022 •

edited

Loading