Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add set request attribute #1080 #1085

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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