search

thriftrw / thriftrw-node

A thrift binary encoding library using bufrw
MIT License
57 stars 25 forks source link

2/4 : Async load #168

Closed dbousque closed 4 years ago

dbousque commented 4 years ago

2/4 : This PR brings support for asynchronous source loading.

claassistantio commented 4 years ago

CLA assistant check
All committers have signed the CLA.

dbousque commented 4 years ago

Tests for the new functionality is missing from this branch, hence the lack of coverage. Take a look at the full PR for async source loading + tests : https://github.com/thriftrw/thriftrw-node/pull/173

kriskowal commented 4 years ago

Let’s collapse these two changes so we can unlock continuous integration. Thank you for breaking these into more reviewable chunks, but we as a rule only land commits that pass tests so we can bisect regressions.

kriskowal commented 4 years ago

Also, please rebase. I fixed some issues on master separately.

dbousque commented 4 years ago

The latests changes from master have been integrated. For the full PR with tests, we should switch to https://github.com/thriftrw/thriftrw-node/pull/173. I can also merge it in this PR if you'd like.

kriskowal commented 4 years ago

Thanks @dbousque. Please consolidate into one landable pull request and close the other. #173 is passing tests but has conflicts with master. It looks like I can stamp and land today, but I won’t be responsive through the rest of the month.

dbousque commented 4 years ago

Thanks @kriskowal, hopefully we can make it today then :) I just merged the tests PR in this one. This PR should be up to date with thriftrw-node:master.

kriskowal commented 4 years ago

Can I ask you to look into the conflicts? Either merge or rebase with master? According to the UI on Github, it’s not yet free of conflicts.

Either way, we are going to squash to land.

dbousque commented 4 years ago

Weird, I don't see that, and I just merged master in the branch 🤔. Maybe I should squash already?

dbousque commented 4 years ago

Apparently there was a mishap on my part, do you still have conflicts now?

kriskowal commented 4 years ago

image This is what I see.

kriskowal commented 4 years ago

I brought it down locally and started a rebase on master. There’s a lot going on here but one of the things is that I added tests while reconciling the last set of changes, which now need to be indented into their new homes.

In any case, we’re under a release freeze until January. I’ll have some time to help out with this later this month.

dbousque commented 4 years ago

@kriskowal I rebased and pushed, we should be conflict free now :)