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] Add Blacklist to Recorder #1641

Open
1 task done
TyrantKingBen opened this issue Mar 24, 2024 · 4 comments
Open
1 task done

[FEAT] Add Blacklist to Recorder #1641

TyrantKingBen opened this issue Mar 24, 2024 · 4 comments

Comments

@TyrantKingBen
Copy link
Contributor

Problem Description

The recorder can filter recorded users by id, acting as a whitelist/allowlist. If you want it to function as a blacklist/denylist, then you are forced to whitelist all guild members minus the desired filtered users. This is very inconvenient, especially considering you would need to have an event handler to update the filter on every guild join event.

Proposed Solution

Refactor the way the recorder does filtering and add a recording mode to allow changing between allowlist and denylist behaviors.

Alternatives Considered

No response

Additional Information

No response

Code of Conduct

  • I agree to follow the contribution requirements.
@AstreaTSS
Copy link
Member

To clarify: it should be as simple as adding an extra field to the class to serve as a blacklist of members (and adding basic logic to filter out said members).

@AstreaTSS AstreaTSS changed the title [FEAT] Add Recording Mode to Recorder [FEAT] Add Blacklist Mode to Recorder Mar 24, 2024
@AstreaTSS AstreaTSS changed the title [FEAT] Add Blacklist Mode to Recorder [FEAT] Add Blacklist to Recorder Mar 24, 2024
@TyrantKingBen
Copy link
Contributor Author

To clarify: it should be as simple as adding an extra field to the class to serve as a blacklist of members (and adding basic logic to filter out said members).

There are two reasons I had for thinking a recording mode is better.

  1. You cannot have a whitelist and a blacklist both being used simultaneously. If someone is not on the whitelist, they are inherently on the blacklist and vice-versa.
  2. The usage could stay the same, namely in start_recording, but also in filter.
    async def start_recording(
        self,
        *user_ids: Snowflake_Type,
        recording_mode: RecordingMode | None = None,
        output_dir: str | None = None,
    ) -> None:
        ...

    def filter(self, *user_ids: Snowflake_Type, recording_mode: RecordingMode | None = None) -> None:
        """
        Filter the users that are being recorded.

        Args:
            *user_ids: The user_ids to either record or not record, depending on the recording mode, if not specified everyone will be included
            recording_mode: The recording mode to use, if not specified the recording list will act as an allowlist

        """
        self.recording_list = to_snowflake_list(unpack_helper(user_ids))

        if recording_mode is not None:
            self.recording_mode = recording_mode

    def is_filtered(self, user_id: Snowflake_Type) -> bool:
        """
        Checks if a user is filtered from being recorded.

        Args:
            user_id: The user_id to check

        Returns:
            True if the user is filtered from being recorded, otherwise False

        """
        match self.recording_mode:
            case RecordingMode.ALLOW:
                return self.recording_list and user_id not in self.recording_list
            case RecordingMode.DENY:
                return not self.recording_list or user_id in self.recording_list
            case _:
                return False

With the lists separate, the only way that start_recording can be changed to support starting in either whitelist or blacklist mode is to add a default parameter which can be used to infer which list to set from the *args. At that point, it may as well be one list with a recording mode as I was originally thinking.

    async def start_recording(
        self,
        *user_ids: Snowflake_Type,
        is_whitelist: bool = True,
        output_dir: str | None = None,
    ) -> None:
        ...

    def set_whitelist(self, *user_ids: Snowflake_Type) -> None:
        self.recording_blacklist = []
        self.recording_whitelist = to_snowflake_list(unpack_helper(user_ids))

    def set_blacklist(self, *user_ids: Snowflake_Type) -> None:
        self.recording_whitelist = []
        self.recording_blacklist = to_snowflake_list(unpack_helper(user_ids))

    def filter(self, *user_ids: Snowflake_Type, is_whitelist: bool = True) -> None:
        if is_whitelist:
            self.set_whitelist(*user_ids)
        else
            self.set_blacklist(*user_ids)

    def is_filtered(self, user_id: Snowflake_Type) -> bool:
        """
        Checks if a user is filtered from being recorded.

        Args:
            user_id: The user_id to check

        Returns:
            True if the user is filtered from being recorded, otherwise False

        """
        return (self.recording_whitelist and user_id not in self.recording_whitelist) or (
            self.recording_blacklist and user_id in self.recording_blacklist
        )

@AstreaTSS
Copy link
Member

Honestly, that's the idea I had (more or less). Though I'd also rather not have the add_black/whitelist to prevent people from running into unexpected behavior (lots of debate can be had if whitelists and blacklists are mutually exclusive in context - in this case it is, but in some it's possible for it not to be).

@TyrantKingBen
Copy link
Contributor Author

Okay, I can inline those and swap things over to using the two lists on my feature branch.

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

No branches or pull requests

2 participants