Add method SetAcceptEncoding for customized Accept-Encoding header (#683) #746

leviliangtw · 2022-05-17T23:41:28Z

New class for managing header Accept-Encoding refers to proxies structure.cpp
New method to set header Accept-Encoding
New unit test to verify header Accept-Encoding

COM8 · 2022-05-19T09:50:56Z

 1: [----------] 5 tests from BasicAuthenticationTests
  1: [ RUN      ] BasicAuthenticationTests.BasicAuthenticationSuccessTest
  1: /home/runner/work/cpr/cpr/test/abstractServer.cpp:108:20: runtime error: left shift of 492624220 by 6 places cannot be represented in type 'int'
  1: HEADERS: Authorization: Basic dXNlcjpwYXNzd29yZA==
  1: User-Agent: curl/7.81.0
  1: Accept-Encoding: deflate, gzip
  1: Content-Type: text/html
  1: [       OK ] BasicAuthenticationTests.BasicAuthenticationSuccessTest (1 ms)
  1: [ RUN      ] BasicAuthenticationTests.BasicBearerSuccessTest
  1: HEADERS: Authorization: ***
  1: User-Agent: curl/7.81.0
  1: Accept-Encoding: deflate, gzip
  1: Content-Type: text/html

COM8

Thanks for looking into this.

COM8 · 2022-05-19T09:51:39Z

cpr/accept_encoding.cpp

+
+AcceptEncoding::AcceptEncoding(const std::initializer_list<AcceptEncodingMethods>& methods) : methods_{methods} {}
+
+bool AcceptEncoding::isEmpty() const {


All public methods have to start with an upper case letter.

I would like to change its name to has, just like the implementation in proxies.cpp:

bool Proxies::has(const std::string& protocol) const { return hosts_.count(protocol) > 0; }

This is also possible.

COM8 · 2022-05-19T09:59:05Z

include/cpr/accept_encoding.h

+
+class AcceptEncoding {
+  public:
+    std::map<AcceptEncodingMethods, std::string> MethodsString { 


Although I like the way you are solving this here, curl allows even more encodings: https://curl.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html

Since this is something that changes based on what is build into curl, in my eyes we need at least an additional option allowing the user to specify their own encoding via a string name.

Sure, I will add an overload function for that to make it more flexible.

COM8

This one goes into the right direction. Only a few things to sort out. Also please rebase your changes on master: https://gist.github.com/ravibhure/a7e0918ff4937c9ea1c456698dcd58aa

COM8 · 2022-05-24T06:25:12Z

test/abstractServer.cpp

@@ -105,7 +105,7 @@ std::string AbstractServer::Base64Decode(const std::string& in) {
            break;
        }
        // NOLINTNEXTLINE (cppcoreguidelines-avoid-magic-numbers)
-        val = (val << 6) + T[c];
+        val = ((uint)val << (uint)6) + T[c];


Please don't use C-style casts

Suggested change

val = ((uint)val << (uint)6) + T[c];

val = (static_cast<uint>(val) << static_cast<uint>(6)) + T[c];

COM8 · 2022-05-24T06:27:11Z

include/cpr/accept_encoding.h

+
+class AcceptEncoding {
+  public:
+    std::map<AcceptEncodingMethods, std::string> MethodsStringMap { 


Can't this map be static and const?

I would investigate it and find a best practice for that

COM8 · 2022-05-24T06:28:43Z

include/cpr/accept_encoding.h

+    AcceptEncoding(const std::initializer_list<AcceptEncodingMethods>& methods);
+    AcceptEncoding(const std::initializer_list<std::string>& methods);
+
+    bool has() const;


I thought about it again and I prefer empty() here as name.

cpr/session.cpp

cpr/accept_encoding.cpp

COM8 · 2022-05-29T07:38:35Z

msvc on Windows reports:

  D:\a\cpr\cpr\cpr\accept_encoding.cpp(8,24): error C2039: 'back_inserter': is not a member of 'std' [D:\a\cpr\cpr\build\cpr\cpr.vcxproj]
  C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.32.31326\include\vector(24): message : see declaration of 'std' [D:\a\cpr\cpr\build\cpr\cpr.vcxproj]
  D:\a\cpr\cpr\cpr\accept_encoding.cpp(8,37): error C3861: 'back_inserter': identifier not found [D:\a\cpr\cpr\build\cpr\cpr.vcxproj]

COM8

Looks good now, thanks!
Please create a PR for a bit of documentation here: https://github.com/libcpr/docs

COM8 · 2022-06-03T08:04:50Z

D:\a\cpr\cpr\test\abstractServer.cpp(108,28): error C2061: syntax error: identifier 'uint' [D:\a\cpr\cpr\build\test\test_server.vcxproj]

…ibcpr#683)

leviliangtw · 2022-06-04T04:39:47Z

D:\a\cpr\cpr\test\abstractServer.cpp(108,28): error C2061: syntax error: identifier 'uint' [D:\a\cpr\cpr\build\test\test_server.vcxproj]

I think I should not modify this file.

Add method SetAcceptEncoding for customized Accept-Encoding header (libcpr#683)

leviliangtw force-pushed the bugfix/session branch 3 times, most recently from 021210a to 68d527b Compare May 18, 2022 00:41

leviliangtw mentioned this pull request May 18, 2022

How to disable post request compression #683

Closed

COM8 requested changes May 19, 2022

View reviewed changes

COM8 added Feature ✨ Documentation Needed 📗 labels May 19, 2022

leviliangtw force-pushed the bugfix/session branch 4 times, most recently from 39f4a0e to e27999b Compare May 22, 2022 20:26

leviliangtw requested a review from COM8 May 23, 2022 09:29

COM8 requested changes May 24, 2022

View reviewed changes

leviliangtw force-pushed the bugfix/session branch 4 times, most recently from 49432fc to 24cbfb7 Compare May 28, 2022 01:08

leviliangtw requested a review from COM8 May 28, 2022 01:09

COM8 approved these changes May 29, 2022

View reviewed changes

leviliangtw force-pushed the bugfix/session branch 2 times, most recently from 04b1cb8 to 1383954 Compare June 1, 2022 06:00

leviliangtw requested a review from COM8 June 1, 2022 06:05

COM8 approved these changes Jun 1, 2022

View reviewed changes

leviliangtw force-pushed the bugfix/session branch from 1383954 to be9ece6 Compare June 2, 2022 11:07

leviliangtw requested a review from COM8 June 2, 2022 11:11

COM8 approved these changes Jun 3, 2022

View reviewed changes

Add method SetAcceptEncoding for customized Accept-Encoding header (l…

1295bba

…ibcpr#683)

leviliangtw force-pushed the bugfix/session branch from be9ece6 to 1295bba Compare June 4, 2022 04:34

leviliangtw requested a review from COM8 June 4, 2022 04:38

COM8 merged commit dd715f6 into libcpr:master Jun 5, 2022

leviliangtw deleted the bugfix/session branch June 5, 2022 13:08

leviliangtw pushed a commit to leviliangtw/cpr that referenced this pull request Jun 5, 2022

Merge pull request libcpr#746 from leviliangtw/bugfix/session

2e9b9ad

Add method SetAcceptEncoding for customized Accept-Encoding header (libcpr#683)

COM8 linked an issue Jun 25, 2022 that may be closed by this pull request

How to disable post request compression #683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method SetAcceptEncoding for customized Accept-Encoding header (#683) #746

Add method SetAcceptEncoding for customized Accept-Encoding header (#683) #746

leviliangtw commented May 17, 2022

COM8 commented May 19, 2022

COM8 left a comment

COM8 May 19, 2022

leviliangtw May 19, 2022

COM8 May 20, 2022

COM8 May 19, 2022

leviliangtw May 19, 2022

COM8 left a comment

COM8 May 24, 2022

leviliangtw May 26, 2022

COM8 May 24, 2022

leviliangtw May 26, 2022

COM8 May 24, 2022

leviliangtw May 26, 2022

COM8 commented May 29, 2022

COM8 left a comment

COM8 commented Jun 3, 2022 •

edited

leviliangtw commented Jun 4, 2022


		AcceptEncoding::AcceptEncoding(const std::initializer_list<AcceptEncodingMethods>& methods) : methods_{methods} {}

		bool AcceptEncoding::isEmpty() const {

	val = ((uint)val << (uint)6) + T[c];
	val = (static_cast<uint>(val) << static_cast<uint>(6)) + T[c];

Add method SetAcceptEncoding for customized Accept-Encoding header (#683) #746

Add method SetAcceptEncoding for customized Accept-Encoding header (#683) #746

Conversation

leviliangtw commented May 17, 2022

COM8 commented May 19, 2022

COM8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

COM8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

COM8 commented May 29, 2022

COM8 left a comment

Choose a reason for hiding this comment

COM8 commented Jun 3, 2022 • edited

leviliangtw commented Jun 4, 2022

COM8 commented Jun 3, 2022 •

edited