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

WIP: Add more decorators #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

stevefan1999-personal
Copy link

@stevefan1999-personal stevefan1999-personal commented Nov 27, 2019

@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

Merging #1 into master will decrease coverage by 11.72%.
The diff coverage is 87.12%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #1       +/-   ##
===========================================
- Coverage     100%   88.27%   -11.73%     
===========================================
  Files           6       22       +16     
  Lines          28      145      +117     
  Branches        5       13        +8     
===========================================
+ Hits           28      128      +100     
- Misses          0       15       +15     
- Partials        0        2        +2
Impacted Files Coverage Δ
src/utils.ts 100% <100%> (ø)
src/decorators/Attribute.ts 100% <100%> (ø) ⬆️
src/decorators/HasMany.ts 100% <100%> (ø)
src/decorators/Field.ts 100% <100%> (ø) ⬆️
src/decorators/BelongsTo.ts 100% <100%> (ø)
src/decorators/Num.ts 100% <100%> (ø) ⬆️
src/decorators/Str.ts 100% <100%> (ø) ⬆️
src/decorators/index.ts 100% <100%> (ø)
src/decorators/Bool.ts 100% <100%> (ø) ⬆️
src/decorators/DecoratedModel.ts 69.23% <69.23%> (ø)
... and 28 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 1c92ed4...4b11226. Read the comment docs.

@kiaking
Copy link
Member

kiaking commented Dec 23, 2019

@stevefan1999-personal Thank you so much for the PR! Wow you've done almost everything 👍 Sorry for my late reply. Do you still want to continue with this one? Or should I take over? (since it has been while so, sorry for that)

@stevefan1999-personal
Copy link
Author

@kiaking I can still be a contributor

@kiaking
Copy link
Member

kiaking commented Dec 23, 2019

@stevefan1999-personal Great! Thanks. I'll make some comments.

describe('Attribute', () => {
it('can define `attr` field', () => {
@suite
export class AttributeSpec {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use non-class format for the test? Just to be consistent with Vuex ORM core.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, but I think it mentally looks better to developers that has adopted to N/JUnit...no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe yes, though most of Vue related libraries are written in function way so rather than Class format so 😓 Or I mean I could change this one at my side though.

@kiaking
Copy link
Member

kiaking commented Dec 24, 2019

@stevefan1999-personal Thanks a lot! Do you have more to add? If not I'm ready to merge this baby!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants