Skip to content

Conversation

@Fabio3rs
Copy link

@Fabio3rs Fabio3rs commented Jul 24, 2022

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!

@andreastedile
Copy link

andreastedile commented Jul 25, 2022

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?).

@Fabio3rs
Copy link
Author

Fabio3rs commented Jul 25, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants