ID Fields

[thgpdr]

Fixes #46

Proposed Changes

Adding ID field option on metadata.

Took the liberty to add a type validation for ID fields (only Number and String), if the wrong type is sent, gotu will ignore the isID option. Hope it's consensus among all that we need something like that, but I did not implemented nothing fancy for now.

Any feedbacks? Thanks

Readiness Checklist

Author/Contributor

• [x] If documentation is needed for this change, has that been included in this pull request
• [x] Remember to check if code coverage decrease, if so, please implement the necessary tests

Reviewing Maintainer

• [ ] Label as breaking if this is a large fundamental change
• [ ] Label as either automation, bug, documentation, enhancement, infrastructure, or performance

[codecov[bot]]

Codecov Report

Merging #48 (59a1d2b) into master (91c62d1) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 59a1d2b differs from pull request most recent head ffce3da. Consider uploading reports for the commit ffce3da to get more accurate results

@@            Coverage Diff            @@
##            master       #48   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         4    +1     
  Lines          103       108    +5     
=========================================
+ Hits           103       108    +5     

Impacted Files	Coverage Δ
src/field.js	100.00% <100.00%> (ø)
src/id.js	100.00% <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 91c62d1...ffce3da. Read the comment docs.

[dalssoft]

I think we are going in the right direction. One point: is the PR name using the correct semantics? https://github.com/herbsjs/gotu/blob/master/.github/CONTRIBUTING.md

[dalssoft]

Something to think about before we put the idea of IDs in the lib: What should the behavior of .validate() be? Ex: If a field is an ID should it allow null (allowNull: false)?

[jhomarolo]

Something to think about before we put the idea of IDs in the lib: What should the behavior of .validate() be? Ex: If a field is an ID should it allow null (allowNull: false)?

For me it's not a problem that the ID is blank/null, because sometimes, in an insertion case, the ID only exists after an insertion in the database.

[dalssoft]

For me it's not a problem that the ID is blank/null, because sometimes, in an insertion case, the ID only exists after an insertion in the database.

Good point. So what should be the DX here?

Scenario 1 - ID should not be null .validate() will fail for new entities (it shouldn't) and it will also fail for others UCs like update for entities with no IDs (it's ok)

Scenario 2 - ID can be null .validate() will not fail for new entities (it's ok) but it will also not fail for others UCs like update for entities with no IDs (it shouldn't)

So, should we have many validators (ex: .validateForUpdate())? Any ideas?

[thgpdr]

I dont know about many validators like.validateForUpdate(). Maybe the responsibility of checking the id presence for a create/update scenario should be more manual, because its very specific.

What about .validate({ checkId: true })?

[dalssoft]

What about .validate({ checkId: true })?

Suggestion: .validate({ exceptIDs: true }) - exceptions should be a opt-in

@thgpdr can you try implement it on this PR?

[thgpdr]

@dalssoft

Suggestion: .validate({ exceptIDs: true }) - exceptions should be a opt-in

nice, I like this one better! What about other fields? Should we give them the exception option too?

@thgpdr can you try implement it on this PR?

sure, no problem!

[jhomarolo]

@thgpdr maybe like this:

.validate().except([ 'id', 'createdAt', 'myOtherProperty']) or .validate( except : [ 'id', 'createdAt', 'myOtherProperty'] )

what do you think?

[dalssoft]

@jhomarolo the reason I like .validate({ exceptIDs: true }) more is because it has a clear use case: Create XYZ use cases. The CLI just need to create the file once and don't have to change it even if the entity changes.

.validate( except : [ 'id', 'createdAt', 'myOtherProperty'] ) might be more flexible, but less declarative. However, It might be useful in the future for more advanced scenarios. For now I would go for exceptIDs

[jhomarolo]

@thgpdr any news about this PR?