pistache icon indicating copy to clipboard operation
pistache copied to clipboard

Add set request attribute #1080

Open Fabio3rs opened this issue 2 years ago • 2 comments

The issue #1080 has some time and I didn't saw pull requests, I think this is very useful, so I make some code that do it.

I created two commits:

  • the code
  • the ninja clang-format that the git commit scripts asks

Suggestions for more tests? Or names? Thanks!

Fabio3rs avatar Jul 24 '22 23:07 Fabio3rs

Hi, I am not a contributor of Pistache, but I can provide the following feedback.

  • getData, tryGetData and getDataAs: I would personally condense these functions into one single function returning a std::optional. The optional should be parametrized with the type of data I want it to hold (just like what getDataAs currently does).
  • I prefer setData instead of putData: as a user, knowing that there is a get function, I would look for a set corresponding one rather than a put one.
  • I am not 100% sure that removeData should throw an error if the key does not exist.
  • In my opinion, the naming of the functions can be improved to reflect their intent. What we want to do is to set and get the attributes of a request: setAttribute and getAttribute are intuitive to me, whereas I would have more trouble understanding the intent of setData and getData (what does "data" refer to? is it perhaps the HTTP body?).

andreastedile avatar Jul 25 '22 16:07 andreastedile

Hi. Thank you! I think your observations are good, I was trying to make similar to the Pistache::Tcp::Peer code https://github.com/pistacheio/pistache/blob/master/include/pistache/peer.h#L54

I am not sure how to proceed is this case.

Fabio3rs avatar Jul 25 '22 20:07 Fabio3rs