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

Fixing some flow types on index.js on isPackageInstalled function #476

Merged
merged 2 commits into from Apr 18, 2019
Merged

Fixing some flow types on index.js on isPackageInstalled function #476

merged 2 commits into from Apr 18, 2019

Conversation

MateusAndrade
Copy link
Collaborator

Overview

Fixing some flowTypes on isPackageInstalled function promise and packageName param.

Test Plan

Clone the PR, and call the isPackageInstalled function.

@mikehardy
Copy link
Collaborator

mikehardy commented Apr 18, 2019

Hi there! Fixing types is awesome, but it's hard for me to say just by looking whether this is valid or not. I mean, developer to developer, this looks obvious - but what if my mind is playing tricks on me?

Could you add a flow check package.json run script (handy example: https://github.com/react-native-community/react-native-device-info/blob/master/package.json#L13) to make sure the result is good, and if it looks like there's a good place to put it in the circle CI config make sure it is called there? Somewhere around here https://github.com/react-native-community/react-native-share/blob/master/circle.yml#L18 should work I think - with the flow-bin being a new devDependency as needed I think...

Most importantly (vs the obvious change here) it should make sure it doesn't regress in the future

@MateusAndrade
Copy link
Collaborator Author

@mikehardy sure! I just got a doubt, or even a suggestion: is there any contrib guide or something like that? Because i got stuck trying to commit my changes with commitlint. And i dont found any doc explaining how should i commit, etc.

@mikehardy
Copy link
Collaborator

Alas, there is not. I remember attempting to send a patch to this library was in fact the first time that I was confronted by my total lack of knowledge of all of react-native (with husky, commitlint, changelog formats, etc etc etc). I'm not sure I'd be qualified to write it either but I know @jgcmarins has recently added some documentation (issue templates etc) and he may have set up the package.json originally - maybe he knows?

@jgcmarins
Copy link
Member

Awesome discussion!
There are few rules here: https://github.com/react-native-community/react-native-share/blob/44ac820e77bc90f331490320a509a52d630272b2/commitlint.config.js
And there is missing a contributing guide explaining how commit lint works.

@MateusAndrade
Copy link
Collaborator Author

@jgcmarins would be really amazing if someone write a guide about commit lint, because i needed to use --no-verify to skip the lint process as there is any example about this

@jgcmarins jgcmarins mentioned this pull request Apr 18, 2019
1 task
@jgcmarins
Copy link
Member

here you go: #477
let's work on this

@jgcmarins jgcmarins merged commit 24a1deb into react-native-share:master Apr 18, 2019
@mikehardy
Copy link
Collaborator

I like that this was merged - but I was hoping for the flow check in CI prior to make sure it is good and doesn't regress later. I did not test it locally

@jgcmarins
Copy link
Member

Agreed, we can add the flow check step on another PR

@MateusAndrade
Copy link
Collaborator Author

@mikehardy i agree with that. I will look at the CI/Flow Check on this weekend =D

@MateusAndrade
Copy link
Collaborator Author

#480

@MateusAndrade MateusAndrade mentioned this pull request May 14, 2019
2 tasks
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

3 participants