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

fix: resolve build issues and update gitignore #11

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

Conversation

24rr
Copy link

@24rr 24rr commented Dec 15, 2024

WIP

What's new in this branch?:

  • Webhook feature
  • fixed build errors
  • (hopefully detailed docs soon)
  • tests (googletest)

@null8626 null8626 self-requested a review December 15, 2024 07:03
Copy link
Member

@null8626 null8626 left a 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)
Copy link
Member

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?

Copy link
Member

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})
Copy link
Member

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?

Copy link
Author

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

Copy link
Member

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

Copy link
Member

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"
Copy link
Member

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!

Copy link
Author

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.
Copy link
Member

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.
Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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) {
Copy link
Member

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) {
Copy link
Member

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>());
Copy link
Member

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!

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