-
Notifications
You must be signed in to change notification settings - Fork 171
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
Backfill ts tests for actual implementations. #268
Comments
@jcchavezs Could you please assign this issue to me? What is the standard PR styles in our project? |
usually we do a PR per task not per file. if all the changes are related I
would raise one pr
…On Tue, 9 Oct 2018, 04:48 Binh Le, ***@***.***> wrote:
@jcchavezs <https://github.com/jcchavezs> Could you please assign this
issue to me?
What is the standard PR styles in our project?
For example, I prefer to write the test for about 10 functions at once and
file by file.
I mean if I create the PR for this issue, I will divide it into multiple
small PRs.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#268 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD61_Z2c1T_ruQZCtBtmddsyD6X4Cyiks5ui7oCgaJpZM4XMgsq>
.
|
@sonybinhle I kind of see your point but what I believe you meant is one PR per package (because hapilly an index.d.ts matches with one package). I am OK with you doing it per package as it will be easy to review an merge. |
@adriancole @jcchavezs Thank you very much, so I will raise the PRs per package. |
Backfill ts tests for actual implementations. #268
After #264 was merged now we have a way to test implementation to fulfill TS signature. What is missing now is to backfill tests for all packages.
Ping @sonybinhle
The text was updated successfully, but these errors were encountered: