From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp20.mail.ru (smtp20.mail.ru [94.100.179.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B0FD3430D56 for ; Tue, 29 Oct 2019 02:42:25 +0300 (MSK) Date: Tue, 29 Oct 2019 02:42:17 +0300 From: Alexander Turenko Message-ID: <20191028234216.akbicvptcgov2j3x@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v4 1/4] http: add CURLOPT_ACCEPT_ENCODING option List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev Cc: tarantool-patches@freelists.org, tarantool-patches@dev.tarantool.org I have only a few comments about texts, the code is okay. WBR, Alexander Turenko. > http: add CURLOPT_ACCEPT_ENCODING option I would use 'httpc:' prefix for http client commits: we did it for several recent patches about http client. I would say that accept_encoding option was added or that CURLOPT_ACCEPT_ENCODING binding is added. > > CURLOPT_ACCEPT_ENCODING option is now supported. > This option enables automatic decompression of HTTP downloads. > See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html > > Closes #4232 > > @TarantoolBot document > Title: http: CURLOPT_ACCEPT_ENCODING option The same comments as above are applicable here too. > Update the documentation for curl options to reflect new > CURLOPT_ACCEPT_ENCODING option. It enables automatic And here. > decompression of HTTP downloads by setting the contents of the > Accept-Encoding header sent in a HTTP request and enabling > decoding of a response when a Content-Encoding header is received. > This is a request, not an order; the server may or may not do it. > Servers might respond with Content-Encoding even without getting > an Accept-Encoding in the request. Servers might respond with a > different Content-Encoding than what was asked for in the request. > @param encoding specifies what encoding you'd like. This param Doxygen-style @param here is a typo? The public http client API only has 'accept_encoding' option, not 'encoding'. > can be an empty string which means Accept-Encoding header will > contain all built-in supported encodings. This param can be > comma-separated list of accepted encodings, like: > "br, gzip, deflate". Bundled libcurl supports "identity", > meaning non-compressed, "deflate" which requests the server to > compress its response using the zlib algorithm and "gzip" which > requests the gzip algorithm. System libcurl also possibly > supports "br" which is brotli. > See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html I suggest to split this block of text by paragraph to ease reading. > +/** > + * Enable automatic decompression of HTTP downloads: set the > + * contents of the Accept-Encoding header sent in a HTTP request > + * and enable decoding of a response when a Content-Encoding > + * header is received. This is a request, not an order; the > + * server may or may not do it. Servers might respond with > + * Content-Encoding even without getting an Accept-Encoding in the > + * request. Servers might respond with a different > + * Content-Encoding than what was asked for in the request. > + * @param req request > + * @param encoding - specify what encoding you'd like. This param > + * can be an empty string which means Accept-Encoding header will > + * contain all built-in supported encodings. This param can be > + * comma-separated list of accepted encodings, like: > + * "br, gzip, deflate". Bundled libcurl supports "identity", > + * meaning non-compressed, "deflate" which requests the server to > + * compress its response using the zlib algorithm and "gzip" which > + * requests the gzip algorithm. System libcurl also possibly > + * supports "br" which is brotli. > + * @see https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html The same: I would split it to meaningful paragraphs to ease reading.