search

herbsjs / gotu

Entities - Create your Entity and enjoy an elegant validation and serialization
Other
11 stars 19 forks source link

changed initial value to null of primitive types #31

Closed m7vicente closed 3 years ago

m7vicente commented 3 years ago

Fixed errors in validation appointed on issue #28

Before, number and string fields as initialized by 0 and '', so, when suma try's validate 0 as presence true they return true, since zero is different then null or undefined

I have changed de value initial value from string and Number to be null as default, from String in specific to keep the coherence, but the comportment keeps the same as before.

codecov[bot] commented 3 years ago

Codecov Report

Merging #31 (6103c80) into master (b982ab1) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #31   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          109       101    -8     
=========================================
- Hits           109       101    -8
Impacted Files Coverage Δ
src/field.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b982ab1...6103c80. Read the comment docs.

thgpdr-vortx commented 3 years ago

I dont know if thats the right solution to the problem. The word 'presence' refers to the existence of a property, in my point of view.

Whats your thoughts about creating another validation option like 'canBeEmpty' or 'emptyOrZero'?

jhomarolo commented 3 years ago

It makes sense to me. But there could be other ways to validate that, perhaps with suma

BritoDoug commented 3 years ago

I think something like @thgpdr-vortx propose make sense, but I also don't see any problem in this implementation

dalssoft commented 3 years ago

Not sure if this is what @thgpdr-vortx is proposing but we already have allowNull:

Presence vs allowNull : https://github.com/herbsjs/suma#presence-vs-allownull

thgpdr-vortx commented 3 years ago

@dalssoft thats what I had in mind. I'd forgot about allowNull and also got it wrong at first the change proposed by @m7vicente

I dont see any problems now

dalssoft commented 3 years ago

If this is the way to go I think Boolean and Array should have the same behavior, right?

m7vicente commented 3 years ago

For those primitive types I agree, but, why not for the baseEntity types? has a specific reason for instancing?

dalssoft commented 3 years ago

@m7vicente including baseEntity as well.

dalssoft commented 3 years ago

Looking the modified tests it seems that the a fundamental behavior is changing. Do you have the same feeling? @m7vicente @jhomarolo @BritoDoug @thgpdr-vortx

BritoDoug commented 3 years ago

Looking the modified tests it seems that the a fundamental behavior is changing. Do you have the same feeling? @m7vicente @jhomarolo @BritoDoug @thgpdr-vortx

Not exactly, the tests where I had feel that, have the description changed too. The point is, we need some tests to the old behavior ? I believe not

undefined X null

the argument about reserve null to who implements the library convinces me

jhomarolo commented 3 years ago

@dalssoft , @m7vicente I do not see a fundamental change in functioning, but a change in concepts used, that we should avoid.

For me, null or undefined in this case is conceptual, but by concept, IMHO we should stick to the undefined and not the null.

dalssoft commented 3 years ago

great, so it seems we agreed to use undefined.

next step: who could test this PR in a real system and analyze the impact?

m7vicente commented 3 years ago

The change seans to won't make any impact, I have made some tests in Vórtx internal applications, seans to be fine.

But on to-do-list, and herb2knex, some tests as broken.

jhomarolo commented 3 years ago

The change seans to won't make any impact, I have made some tests in Vórtx internal applications, seans to be fine.

But on to-do-list, and herb2knex, some tests as broken.

@m7vicente Can you print the error here?

m7vicente commented 3 years ago
jhomarolo commented 3 years ago

@m7vicente I believe that the mistakes there make sense, as they validate exactly what we are changing.

So far, so far so good for me, we just need to change these repositories as well.

dalssoft commented 3 years ago

same as @jhomarolo. it looks good to go!

great job @m7vicente!!!

jhomarolo commented 3 years ago

:tada: This PR is included in version 1.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: