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 more methods to HTTPManager #259

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlphaKeks
Copy link

This adds the following methods to HTTPManager:

  • PUT
  • PATCH
  • DELETE

@Vauff
Copy link
Contributor

Vauff commented Jul 6, 2024

Looks fine to me, the Windows build error is very puzzling. I will ask around.

@@ -62,6 +62,9 @@ class HTTPManager
public:
void GET(const char* pszUrl, CompletedCallback callback, std::vector<HTTPHeader>* headers = nullptr);
void POST(const char* pszUrl, const char* pszText, CompletedCallback callback, std::vector<HTTPHeader>* headers = nullptr);
void PUT(const char* pszUrl, const char* pszText, CompletedCallback callback, std::vector<HTTPHeader>* headers = nullptr);
void PATCH(const char* pszUrl, const char* pszText, CompletedCallback callback, std::vector<HTTPHeader>* headers = nullptr);
void DELETE(const char* pszUrl, const char* pszText, CompletedCallback callback, std::vector<HTTPHeader>* headers = nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're gonna need to pick a different name for this function. Apparently DELETE is already defined as a macro in Windows build environments, leading to Windows builds failing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If DELETE is renamed, the other should probably be renamed as well for consistency's sake? We could also just have a single method that takes the HTTP method as a parameter (either by creating our own enum or using valve's).

Copy link
Contributor

@Vauff Vauff Jul 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever works, GET/POST were only split up because of the extra param. Do be mindful of making compatibility breaks with those existing functions though, they are heavily used in server-specific forks.

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