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

feat: allow timeout-related commands to be used in multiple channels #5402

Merged
merged 20 commits into from
Jun 16, 2024

Conversation

pajlada
Copy link
Member

@pajlada pajlada commented May 18, 2024

This PR changes the behaviour of the following commands:

  • /ban
  • /timeout
  • /untimeout
  • /unban

All of those commands now accept one or more --channel parameters to override which channel the action should take place in.
The --channel parameter accepts a channel ID or channel name with the same syntax as the other "user targets" do (e.g. id:11148817 or pajlada)

examples
Ban user in the chat you're typing in:
/ban weeb123

Ban user in the chat you're typing in, with a reason specified:
/ban weeb123 the ban reason

Ban user in a separate chat, with a reason specified:
/ban --channel pajlada weeb123 the ban reason

Ban user in two separate chats, with a reason specified:
/ban --channel pajlada --channel id:117166826 weeb123 the ban reason

Timeout user in the chat you're typing in:
/timeout weeb123

Timeout user in the chat you're typing in, with a reason specified:
/timeout weeb123 10m the timeout reason

Timeout user in a separate chat, with a reason specified:
/timeout --channel pajlada weeb123 10m the timeout reason

Timeout user in two separate chats, with a reason specified:
/timeout --channel pajlada --channel id:117166826 weeb123 10m the timeout reason

Unban user in the chat you're typing in:
/unban weeb123

Unban user in a separate chat:
/unban --channel pajlada weeb123

Unban user in two separate chats:
/unban --channel pajlada --channel id:117166826 weeb123

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

"from chatting. Reason is optional and will be shown to the target "
"user and other moderators. Use \"/unban\" to remove a ban.";
if (words.size() < 2)
std::vector<PerformChannelAction> actions;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'actions' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<PerformChannelAction> actions;
std::vector<PerformChannelAction> actions = 0;

@pajlada pajlada force-pushed the feat/multi-channel-ban branch from b79ed44 to 2ecb541 Compare May 19, 2024 12:24
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -1,5 +1,7 @@
#pragma once

#include "expected.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'expected.hpp' file not found [clang-diagnostic-error]

#include "expected.hpp"
         ^

tests/src/Commands.cpp Show resolved Hide resolved

} // namespace

TEST(Commands, parseBanCommand)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "TEST" is directly included [misc-include-cleaner]

tests/src/Commands.cpp:8:

+ #include <gtest/gtest.h>

tests/src/Commands.cpp Show resolved Hide resolved
};
auto oActions = commands::parseChannelAction(ctx, "/ban", "usage", false);

ASSERT_TRUE(oActions.has_value()) << oActions.error();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "ASSERT_TRUE" is directly included [misc-include-cleaner]

    ASSERT_TRUE(oActions.has_value()) << oActions.error();
    ^


ASSERT_TRUE(oActions.has_value()) << oActions.error();
auto actions = *oActions;
ASSERT_EQ(actions.size(), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "ASSERT_EQ" is directly included [misc-include-cleaner]

    ASSERT_EQ(actions.size(), 1);
    ^

@jupjohn
Copy link
Contributor

jupjohn commented May 21, 2024

In your example, you're using raw IDs but in other commands IDs have to be prefixed with id:. Could the same behaviour be used here to support both channel names & IDs? Keeps it consistent.

Also, are ---prefixed arguments used anywhere else? I don't have a better idea, but they feel very programmer-y which might feel weird to users.

@pajlada
Copy link
Member Author

pajlada commented May 21, 2024

-- prefixes are used in /openurl - I'm ok with them being a bit programmery since this use case is a bit programmery

I'm fine with --channel accepting the same type of input, e.g. 11148817 or pajlada

@pajlada pajlada changed the title feat: allow /ban to be used in multiple channels at once feat: allow timeout-related commands to be used in multiple channels May 21, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -119,189 +121,269 @@

namespace chatterino::commands {

QString sendBan(const CommandContext &ctx)
nonstd::expected<std::vector<PerformChannelAction>, QString> parseChannelAction(
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "nonstd::expected_lite::expected" is directly included [misc-include-cleaner]

src/controllers/commands/builtin/twitch/Ban.cpp:7:

- #include "providers/twitch/api/Helix.hpp"
+ #include "nonstd/expected.hpp"
+ #include "providers/twitch/api/Helix.hpp"

@@ -119,189 +121,269 @@

namespace chatterino::commands {

QString sendBan(const CommandContext &ctx)
nonstd::expected<std::vector<PerformChannelAction>, QString> parseChannelAction(
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::vector" is directly included [misc-include-cleaner]

src/controllers/commands/builtin/twitch/Ban.cpp:14:

+ #include <vector>

{
return "";
// A ban action must be performed with a channel as a context
return nonstd::make_unexpected(
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "nonstd::expected_lite::make_unexpected" is directly included [misc-include-cleaner]

        return nonstd::make_unexpected(
                       ^

}

if (twitchChannel == nullptr)
QCommandLineParser parser;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "QCommandLineParser" is directly included [misc-include-cleaner]

src/controllers/commands/builtin/twitch/Ban.cpp:14:

+ #include <qcommandlineparser.h>


} // namespace

TEST(Commands, parseBanCommand)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "TEST" is directly included [misc-include-cleaner]

tests/src/Commands.cpp:8:

+ #include <gtest/gtest.h>

tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
};
auto oActions = commands::parseChannelAction(ctx, "/ban", "usage", false);

ASSERT_TRUE(oActions.has_value()) << oActions.error();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "ASSERT_TRUE" is directly included [misc-include-cleaner]

    ASSERT_TRUE(oActions.has_value()) << oActions.error();
    ^


ASSERT_TRUE(oActions.has_value()) << oActions.error();
auto actions = *oActions;
ASSERT_EQ(actions.size(), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "ASSERT_EQ" is directly included [misc-include-cleaner]

    ASSERT_EQ(actions.size(), 1);
    ^

pajlada added 4 commits June 2, 2024 11:50
example:
`/ban --channel 11148817 --channel 117166826 forsen game complainer`

this would ban forsen with the reason `game complainer` in the two
listed channels
@pajlada pajlada force-pushed the feat/multi-channel-ban branch from 520e12f to e03c5e8 Compare June 2, 2024 09:50
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/controllers/commands/builtin/twitch/Ban.cpp Outdated Show resolved Hide resolved
src/controllers/commands/builtin/twitch/Ban.cpp Outdated Show resolved Hide resolved
src/controllers/commands/builtin/twitch/Ban.cpp Outdated Show resolved Hide resolved
src/controllers/commands/builtin/twitch/Ban.cpp Outdated Show resolved Hide resolved
tests/src/Commands.cpp Outdated Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Outdated Show resolved Hide resolved
tests/src/Commands.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 29. Check the log or trigger a new build to see more.

tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 29. Check the log or trigger a new build to see more.

tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
@pajlada pajlada marked this pull request as ready for review June 2, 2024 14:31
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

mocks/include/mocks/EmptyApplication.hpp Show resolved Hide resolved
mocks/include/mocks/EmptyApplication.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
mocks/include/mocks/Logging.hpp Show resolved Hide resolved
mocks/include/mocks/Logging.hpp Outdated Show resolved Hide resolved
mocks/include/mocks/Logging.hpp Show resolved Hide resolved
src/singletons/Logging.hpp Show resolved Hide resolved
tests/src/Commands.cpp Show resolved Hide resolved
Copy link
Member Author

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

image

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

mocks/include/mocks/EmptyApplication.hpp Show resolved Hide resolved
mocks/include/mocks/EmptyApplication.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
mocks/include/mocks/LinkResolver.hpp Show resolved Hide resolved
LinkResolver() = default;
~LinkResolver() override = default;

MOCK_METHOD(void, resolve, (LinkInfo * info), (override));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for function 'MOCK_METHOD' [readability-identifier-naming]

Suggested change
MOCK_METHOD(void, resolve, (LinkInfo * info), (override));
mockMethod(void, resolve, (LinkInfo * info), (override));


namespace chatterino::mock {

class Logging : public ILogging
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'Logging' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class Logging : public ILogging
      ^

Logging() = default;
~Logging() override = default;

MOCK_METHOD(void, addMessage,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for function 'MOCK_METHOD' [readability-identifier-naming]

Suggested change
MOCK_METHOD(void, addMessage,
mockMethod(void, addMessage,

(override));
};

class EmptyLogging : public ILogging
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'EmptyLogging' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class EmptyLogging : public ILogging
      ^

@@ -16,13 +16,22 @@ struct Message;
using MessagePtr = std::shared_ptr<const Message>;
class LoggingChannel;

class Logging
class ILogging
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'ILogging' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class ILogging
      ^


using namespace chatterino;

using ::testing::_;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: declaration uses identifier '_', which is reserved in the global namespace; cannot be fixed automatically [bugprone-reserved-identifier]

using ::testing::_;
                 ^

@pajlada pajlada merged commit 9b31246 into master Jun 16, 2024
17 checks passed
@pajlada pajlada deleted the feat/multi-channel-ban branch June 16, 2024 10:22
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