> Looks good - even the transcoding stuff seems to work :) > > Some remarks (nothing blocking): > - It would be nice to keep fetch-binary functions together in the module. fixed
> - Is internal::StreamResource::create ever used with 3 parameters? for http stuff when coming from the api > - fetch_impl.cpp: > - 104 > Why is lEncodingStr initialized if it is always overwritten in line 115? > Also, as the lEncodingStr is only accessed using c_str(), the zstring is > probably not necessary at all. fixed > - 115 > It seems that dynamic_cast<internal::StreamResource*> and the error > handling > could be moved into getFetchResource. fixed > - It would be great to have at least one http-Test (but of course that adds > flakiness to the test run …) not fixed for this reason -- https://code.launchpad.net/~zorba-coders/zorba/feature-fetch_binary/+merge/105497 Your team Zorba Coders is subscribed to branch lp:zorba. -- Mailing list: https://launchpad.net/~zorba-coders Post to : zorba-coders@lists.launchpad.net Unsubscribe : https://launchpad.net/~zorba-coders More help : https://help.launchpad.net/ListHelp
