zorba-coders team mailing list archive

[Till Westmann <till.westmann@xxxxxxxxxx>]

Review: Approve

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.
- Is internal::StreamResource::create ever used with 3 parameters?
- 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.
  - 115
    It seems that dynamic_cast<internal::StreamResource*> and the error handling 
    could be moved into getFetchResource.
- It would be great to have at least one http-Test (but of course that adds 
  flakiness to the test run …)

-- 
https://code.launchpad.net/~zorba-coders/zorba/feature-fetch_binary/+merge/105497
Your team Zorba Coders is subscribed to branch lp:zorba.