Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add uid attribute #16

Merged
merged 4 commits into from
May 15, 2020
Merged

feat: add uid attribute #16

merged 4 commits into from
May 15, 2020

Conversation

kiaking
Copy link
Member

@kiaking kiaking commented May 14, 2020

Add UID attribute.

Type of PR:

  • Bugfix
  • Feature
  • Refactor
  • Code style update
  • Build-related changes
  • Test
  • Documentation
  • Other, please describe:

Details

I had to modify few parts.

Model constructor

Previously, we were simply instantiating model when registering it to the database, or using in repository. Instead, now model accepts null as the constructor argument. When null is passed, all field value generation will be skipped.

Usually, users should never pass null. But pre-registered models don't require fields. It's better for performance, but also it avoids calling uuid multiple times making it hard to mock inside the test. Maybe we could add helper static function to model to create a new model instance without field, because we might for get to pass null in the future when creating new attributes.

Test

This is not new to this PR, but seems like we have to clear the model field registries on each test if we want to declare the same class in multiple test cases. For this, we can use Model.clearRegistries(). Note that we should not use this method for the test where we define class once and use same model in multiple tests. It is only for tests that define multiple same models.

@kiaking kiaking added the enhancement New feature or request label May 14, 2020
@kiaking kiaking requested a review from cuebit May 14, 2020 15:22
@kiaking kiaking self-assigned this May 14, 2020
@codecov
Copy link

codecov bot commented May 14, 2020

Codecov Report

Merging #16 into master will decrease coverage by 0.24%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
- Coverage   96.68%   96.44%   -0.25%     
==========================================
  Files          32       34       +2     
  Lines         754      787      +33     
  Branches      107      113       +6     
==========================================
+ Hits          729      759      +30     
- Misses         24       27       +3     
  Partials        1        1              
Impacted Files Coverage Δ
src/model/Model.ts 89.68% <95.23%> (-0.15%) ⬇️
src/index.ts 100.00% <100.00%> (ø)
src/model/attributes/types/Uid.ts 100.00% <100.00%> (ø)
src/model/decorators/attributes/types/Uid.ts 100.00% <100.00%> (ø)
src/plugin/Plugin.ts 100.00% <100.00%> (ø)
src/query/Query.ts 94.70% <100.00%> (ø)
src/repository/Repository.ts 100.00% <100.00%> (ø)
src/schema/Schema.ts 97.56% <100.00%> (+1.26%) ⬆️
src/model/attributes/relations/HasOne.ts 94.11% <0.00%> (-5.89%) ⬇️
src/model/attributes/relations/HasMany.ts 94.11% <0.00%> (-5.89%) ⬇️
... and 4 more

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 d4c7765...e2f9e97. Read the comment docs.

@kiaking kiaking merged commit e676dd1 into master May 15, 2020
@kiaking kiaking deleted the uid-attribute-type branch May 15, 2020 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants