-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: resolve build issues and update gitignore #11
base: main
Are you sure you want to change the base?
Conversation
- fixes infrastructure/dependency issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hai! There are sadly still many issues I have with this pull request. Most of which are docstrings related, others are code that still needs more polishing or consistency. Thanks buffer!
LANGUAGES CXX | ||
HOMEPAGE_URL "https://docs.top.gg/docs" | ||
DESCRIPTION "The official C++ wrapper for the Top.gg API." | ||
) | ||
|
||
set(CMAKE_BUILD_TYPE Debug CACHE STRING "Build type") | ||
option(BUILD_SHARED_LIBS "Build shared libraries" ON) | ||
option(ENABLE_CORO "Support for C++20 coroutines" OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove ENABLE_CORO
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option lets users of this library choose whether to utilize C++20 coroutines or not (done through callbacks, but compatible with C++17).
@@ -1,15 +1,41 @@ | |||
if(WIN32 AND NOT EXISTS ${CMAKE_SOURCE_DIR}/deps/dpp.lib) | |||
string(TOLOWER ${CMAKE_BUILD_TYPE} INSTALL_DPP_BUILD_TYPE) | |||
execute_process(COMMAND powershell "-NoLogo" "-NoProfile" "-File" ".\\install_dpp_msvc.ps1" ${INSTALL_DPP_BUILD_TYPE} WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The install_dpp_msvc.ps1
script here automatically installs the D++ library for Microsoft Visual C++ users. Why remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: I removed every DPP-related installation scripts because I cannot and don't know how to build it using that on Windows. It shows too much build errors when I first ran it, I don't know if I did something wrong or something. That was the first time I cloned the repository btw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? but it worked for me...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show me what the errors are?
#include <vector> | ||
#include <string> | ||
#include <map> | ||
#include "topgg/export.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should've been #include <topgg/export.h>
. Not that big of an issue, but this would've made it consistent!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
|
||
/** | ||
* @brief The account's Discord ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the docstrings of this class' properties?
|
||
/** | ||
* @brief The Discord bot's discriminator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the docstrings of this class' properties?
} | ||
|
||
friend class client; | ||
}; | ||
|
||
#ifdef DPP_CORO | ||
/** | ||
* @brief An async result class that gets returned from every C++20 coroutine HTTP response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the docstrings of this class' properties?
* @brief Data received from a bot vote webhook | ||
*/ | ||
struct bot_vote_data { | ||
dpp::snowflake user_id; ///< ID of the user who voted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These docstrings should've used the proper format consistent with the other classes in this library (e.g: use @brief
, @version
, etc).
bool is_weekend; ///< Whether the vote was done during weekend | ||
std::string query; ///< Query string data if any | ||
|
||
static bot_vote_data from_json(const nlohmann::json& j) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function body should be moved to webhook.cpp
as it does not depend on any generics.
: cluster_(cluster), port_(port), auth_token_(auth_token) { | ||
|
||
// Set up interaction handler for webhooks | ||
cluster_.on_interaction_create([this](const dpp::interaction_create_t& event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function body should be moved to webhook.cpp
as it does not depend on any generics.
|
||
static bot_vote_data from_json(const nlohmann::json& j) { | ||
bot_vote_data data; | ||
data.user_id = dpp::snowflake(j["user"].get<std::string>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using those DESERIALIZE()
macros!
WIP
What's new in this branch?:
Webhook
feature