Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Ilya Kosarev <i.kosarev@tarantool.org>
Cc: tarantool-patches@freelists.org, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v4 1/4] http: add CURLOPT_ACCEPT_ENCODING option
Date: Tue, 29 Oct 2019 02:42:17 +0300	[thread overview]
Message-ID: <20191028234216.akbicvptcgov2j3x@tkn_work_nb> (raw)
In-Reply-To: <ba1c2c99566a1f0c59a1e5d1913c871a94a1dc6e.1572282336.git.i.kosarev@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.

  reply	other threads:[~2019-10-28 23:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-28 17:11 [Tarantool-patches] [PATCH v4 0/4] http: add CURLOPT_ACCEPT_ENCODING option and following improvements Ilya Kosarev
2019-10-28 17:11 ` [Tarantool-patches] [PATCH v4 1/4] http: add CURLOPT_ACCEPT_ENCODING option Ilya Kosarev
2019-10-28 23:42   ` Alexander Turenko [this message]
2019-10-28 17:11 ` [Tarantool-patches] [PATCH v4 2/4] http: remove redundant & incorrect case for curl_request code Ilya Kosarev
2019-10-28 23:42   ` Alexander Turenko
2019-10-28 17:11 ` [Tarantool-patches] [PATCH v4 3/4] http: enrich httpc_request with curl error message buffer Ilya Kosarev
2019-10-28 23:41   ` Alexander Turenko
2019-11-07 12:07     ` Ilya Kosarev
2019-10-28 17:11 ` [Tarantool-patches] [PATCH v4 4/4] http: add CURLE_BAD_CONTENT_ENCODING case for curl_request code Ilya Kosarev
2019-10-28 23:42   ` Alexander Turenko
2019-10-28 23:43 ` [Tarantool-patches] [PATCH v4 0/4] http: add CURLOPT_ACCEPT_ENCODING option and following improvements Alexander Turenko
2019-11-07 12:07   ` Ilya Kosarev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191028234216.akbicvptcgov2j3x@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=i.kosarev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [Tarantool-patches] [PATCH v4 1/4] http: add CURLOPT_ACCEPT_ENCODING option' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox