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

refactor(filters): checkMaxDownloads #1285

Merged
merged 3 commits into from
Dec 25, 2023
Merged

Conversation

frrad
Copy link
Contributor

@frrad frrad commented Nov 27, 2023

Just some minor style thing I ran into while researching autobrr/autobrr.com#147 feel free to ignore if you don't like it.

Two related questions:

  1. That comment on L264 - why do we return early? All the other possible rejections just add to the list and rely on the if len(...) >0 at the end. I thought that was mysterious.
  2. Also curious about the "current time period" approach as opposed to a lookback window from now. I was thinking about adding a comment here https://github.com/autobrr/autobrr/blob/develop/internal/database/filter.go#L1416 about what the SQL queries are doing, sort of analogous to more explanation for "max per" autobrr.com#147. Would be interested in adding the rationale in the comment too.

@zze0s zze0s changed the title refactor checkMaxDownloads refactor(filters); checkMaxDownloads Dec 1, 2023
@zze0s zze0s changed the title refactor(filters); checkMaxDownloads refactor(filters): checkMaxDownloads Dec 1, 2023
@zze0s zze0s added backend Backend filters Filters labels Dec 1, 2023
@zze0s zze0s added this to the v1.35.0 milestone Dec 1, 2023
Copy link
Collaborator

@zze0s zze0s left a comment

Choose a reason for hiding this comment

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

This is indeed a bit more clean.

  1. We return early because there's no point IMO to process further since no other check will change the decision.
  2. It could for sure use some better comments there. We modeled this like autodl-irssi did and there it was "time window" from start of the unit. It was rolling in the beginning because of bugs. I think it would be confusing and would lead to weird results.

The plan for later down the road is to build a more robust schedule where you can set it per hour similar to this in Deluge:

Pasted image 20210915171027

@zze0s zze0s merged commit d898b3c into autobrr:develop Dec 25, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend filters Filters
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants