Skip to content

Improve typings for computedFn#206

Merged
danielkcz merged 1 commit intomobxjs:masterfrom
mixail-novikov:master
Jun 13, 2019
Merged

Improve typings for computedFn#206
danielkcz merged 1 commit intomobxjs:masterfrom
mixail-novikov:master

Conversation

@mixail-novikov
Copy link
Copy Markdown
Contributor

I notice that computedFn does not infer types from passed callback. I've been fix it, please review.

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 12, 2019

Coverage Status

Coverage remained the same at 94.118% when pulling e98b510 on mixail-novikov:master into fe9b9ec on mobxjs:master.

@danielkcz
Copy link
Copy Markdown

danielkcz commented Jun 12, 2019

I wonder why there are so many differences. Are you using other auto formatter than Prettier? Can you try to do a change without any formatting changes, please?

@xaviergonz
Copy link
Copy Markdown
Contributor

I think what is actually wrong is the setting on prettierrc
https://github.com/mobxjs/mobx-utils/blob/master/.prettierrc

it establishes 4 spaces, while the file is obviously formatted as 2

Maybe there should be a commit just running prettier over all files previous to this commit to ensure consistency?

Copy link
Copy Markdown
Contributor

@xaviergonz xaviergonz left a comment

Choose a reason for hiding this comment

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

lgtm

@danielkcz
Copy link
Copy Markdown

danielkcz commented Jun 12, 2019

@xaviergonz Do you see why the coverage drop? If it's about fixing types only, it shouldn't affect coverage, right? I am failing to see it in that reformatted code.

I agree we should fix prettier and probably add Husky.

@mixail-novikov
Copy link
Copy Markdown
Contributor Author

Thank you for the fast review! I've made changes, please review it again.

Comment thread src/computedFn.ts
@danielkcz danielkcz merged commit 717ad1f into mobxjs:master Jun 13, 2019
@dgreensp
Copy link
Copy Markdown

dgreensp commented Aug 7, 2019

I'd love to see this released!

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.

5 participants