`Validate.IParamCollection.forAll` #88

omaus · 2024-02-22T17:43:09Z

This PR

adds the forAll function in Validate.IParamCollection module/type
also adds an ErrorMessage.ofIParamCollection function according to this
- provides a unit test for this functionality

Closes #72.

kMutagene · 2024-02-23T07:27:50Z

src/ARCExpect/ErrorMessage.fs

+
+    static member ofIParamCollection error iParamCollection =
+
+        let iParam = Seq.head iParamCollection


I do not get these functions to be honest. The API naming indicates that it generates error messages from IParam collections, but you only use the first item in the sequence to generate the error message

Yes, since printing all items would be too much. I mean, this is arbitrary. How many items do we want to display? I chose 1, but we could also do 3 or 5 or any number...
What's your opinion on this?

I mean, this is arbitrary.

Not really, ideally you want to know all items that do not satisfy the predicate

kMutagene · 2024-02-23T07:36:01Z

src/ARCExpect/ValidationFunctions.fs

+            match Seq.forall projection paramCollection with
+            | true  -> ()
+            | false ->
+                ErrorMessage.ofIParamCollection $"does not exist" paramCollection


"does not exist" is not the correct error message here i think. since this should check wether the projection is true for all values, the same message as ValueSatisfiesPredicate ("is invalid) makes more sense.

However, there are 2 more issues here:

please rename to ValuesSatisfyPredicate, as that is the name of the respective function for a single IParam. That way, it becomes Validate.ParamCollection .ValuesSatisfyPredicate ("Validate (for a) ParamCollection (that the) values satisfy (the) predicate")

the error message seems to make even less sense to me combined with the new ErrorMessage.ofIParamCollection, since that uses the head of the sequence. Imagine the second value does not satisfy the predicate - but the error message uses the first - this is very confusing.

It's just not about the Value but about the whole IParam. So the naming should be ParamsSatisfyPredicate.

To the second point: What would you think about reformulating the function so that it does not use Seq.forAll but iterates through the seq so that we can catch the failing IParams and return them?

It's just not about the Value but about the whole IParam

Ideally we would have all 3 variants for Param and ParamCollection.

Thinking again about this, i guess the names of the colelction versions should be a little different.

My suggestions would be

Param.SatisfiesPredicate

Param.TermSatisfiesPredicate

Param.ValueSatisfiesPredicate

ParamCollection.ItemsSatisfyPredicate

ParamCollection.TermsSatisfyPredicate

ParamCollection.ValueySatisfyPredicate

kMutagene · 2024-02-29T07:43:55Z

@omaus how do you intend to proceed on this? for reference, the new API design guidelines are here: https://nfdi4plants.github.io/arc-validate/ARCExpect/design.html.

Do you intend to start with a fresh PR or adapt the code in this one?

omaus · 2024-02-29T07:51:18Z

@omaus how do you intend to proceed on this? for reference, the new API design guidelines are here: https://nfdi4plants.github.io/arc-validate/ARCExpect/design.html.

Do you intend to start with a fresh PR or adapt the code in this one?

This is just about renaming, isn't it? So it should be much easier to just rename the functions instead of writing everything anew.

See: https://nfdi4plants.github.io/arc-validate/ARCExpect/design.html

omaus · 2024-03-07T14:40:26Z

I updated the function's name to fit the naming design and also changed it in a way that the first param that does not satisfy the predicate is shown instead of a part of the whole collection.

kMutagene · 2024-03-07T14:42:28Z

src/ARCExpect/ValidationFunctions.fs

+        /// </summary>
+        /// <param name="projection">A function that evaluates to true if the element satisfies the requirements.</param>
+        /// <param name="paramCollection">The IParam collection to validate.</param>
+        static member ParamsSatisfyPredicate (predicate : #IParam -> bool) (paramCollection : #seq<#IParam>) =


Pls rename to AllItemsSatisfyPredicate as of https://nfdi4plants.github.io/arc-validate/ARCExpect/design.html

(also xml docs and function parameters do not match)

See: https://nfdi4plants.github.io/arc-validate/ARCExpect/design.html

omaus · 2024-03-07T15:01:54Z

and also changed it in a way that the first param that does not satisfy the predicate is shown instead of a part of the whole collection.

Btw. is this okay or would you prefer to show all predicate failing items?

omaus · 2024-03-08T16:57:13Z

and also changed it in a way that the first param that does not satisfy the predicate is shown instead of a part of the whole collection.

Btw. is this okay or would you prefer to show all predicate failing items?

Okay, tweaked it so that it goes throw the whole collection.

kMutagene · 2024-03-11T08:57:07Z

src/ARCExpect/ValidationFunctions.fs

@@ -165,6 +165,7 @@ module Validate =
                    if predicate en.Current |> not then
                        ErrorMessage.ofIParam $"does not satisfy predicate" en.Current
                        |> Expecto.Tests.failtestNoStackf "%s"
+                        loop ()


Have you tested wether this continues after 1 failtestNoStackf is executed? If that throws an exception i am quite sure that it wont

You are correct. It is more complicated to do this than I thought...
But I think I'll get a solution.

kMutagene · 2024-04-19T12:10:29Z

src/ARCExpect/ErrorMessage.fs

+        str.ToString()
+
+
+    static member ofIParamCollection error iParamCollection =


This is not used anymore in the current state of the PR is it?

omaus added 4 commits February 21, 2024 18:27

Add ErrorMessage for IParam collections

bbc013d

Add forAll function to validate IParam collections

1250e91

Closes #72.

Add unit test for ErrorMessage (WIP)

bd4d2de

Add unit test for new ErrorMessage function

722f808

omaus requested a review from kMutagene February 22, 2024 17:43

kMutagene reviewed Feb 23, 2024

View reviewed changes

kMutagene requested changes Feb 23, 2024

View reviewed changes

kMutagene mentioned this pull request Feb 23, 2024

Add PRIDE validation package nfdi4plants/arc-validate-package-registry#16

Merged

omaus added 2 commits March 7, 2024 15:36

Rename function to better match naming design

ef46838

See: https://nfdi4plants.github.io/arc-validate/ARCExpect/design.html

Update function to call first predicate failing param

51030b3

kMutagene requested changes Mar 7, 2024

View reviewed changes

omaus added 2 commits March 7, 2024 15:58

Rename function according to naming design

b008d98

See: https://nfdi4plants.github.io/arc-validate/ARCExpect/design.html

Update XML tags according to renaming

bc3d4aa

Let function failTest all unsatisfying items

528155d

omaus requested a review from kMutagene March 8, 2024 16:57

kMutagene requested changes Mar 11, 2024

View reviewed changes

Update function to display all failing items at once

573dd08

omaus requested a review from kMutagene March 11, 2024 16:22

Delete missed line

9d46428

omaus mentioned this pull request Mar 21, 2024

Add SatisfiesPredicate to Validate.Param and Validate.ParamCollection #91

Merged

kMutagene requested changes Apr 19, 2024

View reviewed changes

omaus requested a review from kMutagene April 23, 2024 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Validate.IParamCollection.forAll` #88

`Validate.IParamCollection.forAll` #88

omaus commented Feb 22, 2024

kMutagene Feb 23, 2024

omaus Feb 23, 2024

kMutagene Feb 26, 2024

kMutagene Feb 23, 2024

omaus Feb 23, 2024

kMutagene Feb 26, 2024 •

edited

Loading

kMutagene commented Feb 29, 2024

omaus commented Feb 29, 2024

omaus commented Mar 7, 2024

kMutagene Mar 7, 2024

kMutagene Mar 7, 2024

omaus Mar 7, 2024

omaus commented Mar 7, 2024

omaus commented Mar 8, 2024

kMutagene Mar 11, 2024

omaus Mar 11, 2024

kMutagene Apr 19, 2024

omaus Apr 23, 2024


		static member ofIParamCollection error iParamCollection =

		let iParam = Seq.head iParamCollection

		str.ToString()


		static member ofIParamCollection error iParamCollection =

Validate.IParamCollection.forAll #88

Are you sure you want to change the base?

Validate.IParamCollection.forAll #88

Conversation

omaus commented Feb 22, 2024

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

kMutagene Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

kMutagene commented Feb 29, 2024

omaus commented Feb 29, 2024

omaus commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omaus commented Mar 7, 2024

omaus commented Mar 8, 2024

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

`Validate.IParamCollection.forAll` #88

`Validate.IParamCollection.forAll` #88

kMutagene Feb 26, 2024 •

edited

Loading