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

Use a blacklist for falsely detected activities #231

Closed
wants to merge 1 commit into from

Conversation

Kyrela
Copy link

@Kyrela Kyrela commented Dec 3, 2024

This simple PR adds a list that contains detected activities that can't be completed (or where it isn't applicable), just like "Safeguard your family's info".

@cal4
Copy link
Collaborator

cal4 commented Dec 3, 2024

Good idea, but this is somewhat implemented already. See

if (
CONFIG.get("apprise")
.get("notify")
.get("incomplete-activity")
.get("ignore-safeguard-info")
):
incompleteActivities.pop("Safeguard your family's info", None)
The main difference here is that the ignorable activities are configurable (with ignore by default).

At the time, I thought there might be some value in allowing the ignores to be configurable but now that I think about it more there doesn't seem to be any value to this. What do you think?

@Kyrela
Copy link
Author

Kyrela commented Dec 3, 2024

Sorry I didn't saw it. Looks like It's only configurable for notifications. Not very usefull imo but hey, you never know. The only problems I have with the current implementation are:

  • Hard-coded: The config rely on ignore-safeguard-info for this specific activity. I think something like
    incomplete-activity:
      enabled: True
      blacklist:
        - "Safeguard your family's info"
    
    would be more dynamic - doesn't require some code changes to ignore another activity not listed by the program
  • Duplicated: The Current implementation ignores the activity in all cases, even if the ignore-safeguard-info is set to False. Seems perfectly logical, but requires two different lists: one that is static, and the other that is configurable. I thnik that it would be simpler if the activities to ignore were just based of the notification configuration - that'll make it a generic configuration and not specifically related to notifications. Don't know if this can be done without problems. But, if we keep the two logics separated, my PR technically still stands, I just had to add notification configuration for this.

Anyway, I'll try to come up with something else shortly, as a proposition. Probably something that's configuration based. Closing this, I'll open another PR, thanks for the feedback!

@Kyrela Kyrela closed this Dec 3, 2024
@cal4
Copy link
Collaborator

cal4 commented Dec 3, 2024

Yeah I agree, a list of titles to ignore is more flexible. I'd recommend incomplete-activity.ignore as opposed to incomplete-activity.blacklist since it reads a bit better IMO.

This would deprecate ignore-safeguard-info, which we should remove of course.

@cal4
Copy link
Collaborator

cal4 commented Dec 9, 2024

Made this change here: 2bf51d3

@Kyrela Kyrela deleted the blacklist branch December 10, 2024 09:19
@Kyrela
Copy link
Author

Kyrela commented Dec 25, 2024

Anyway, I'll try to come up with something else shortly, as a proposition. Probably something that's configuration based. Closing this, I'll open another PR, thanks for the feedback!

Done, see #248

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