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

bugfix: collect decorated class properties #254

Merged
merged 2 commits into from Oct 9, 2018
Merged

Conversation

kuitos
Copy link
Contributor

@kuitos kuitos commented Jun 11, 2018

should collect class properties which decorated by 3rd typescript written decorators library.

Check the added unit tests for details :)

Copy link
Member

@ktsn ktsn left a comment

Choose a reason for hiding this comment

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

Looks like the diff does not respect existing code format. Can you tidy it up?

  • Need a space after if keyword.
  • Use space instead of tab for indentation.

@decorator2('field2')
field2

foo
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 remove this prop to only focus what we want to test in this block?

test/test.ts Outdated
@@ -60,6 +60,48 @@ describe('vue-class-component', () => {
expect(c.b).to.equal(2)
})

it('data: should collect from decorated class properties', () => {

const decorator1 = (value: any) => (_: any, __:any ): any => {
Copy link
Member

Choose a reason for hiding this comment

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

Please use more descriptive name like valueDecorator, getterDecorator.

test/test.ts Outdated
}
}

const decorator2 = (value: any) => (_: any, __:any ): any => {
Copy link
Member

Choose a reason for hiding this comment

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

(_: any, __:any ): any

Please fix the position of space. (add a space after second colon and remove the space before closing brace)
Looks like there is a kind of things in other place.

@kuitos
Copy link
Contributor Author

kuitos commented Jun 21, 2018

@ktsn fixed 👌

@ktsn ktsn merged commit 555e78a into vuejs:master Oct 9, 2018
@ktsn
Copy link
Member

ktsn commented Oct 9, 2018

Thank you for your contribution 🙂

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