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

ENH: implement cleandb feature #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sciunto
Copy link

@sciunto sciunto commented Jun 14, 2015

Hi,

In this PR, i propose a new function to solve this issue: #33
It basically combines reset and run functions.

Thank you!

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@wking
Copy link
Owner

wking commented Dec 12, 2015

On Sun, Jun 14, 2015 at 06:21:16AM -0700, François Boulogne wrote:

In this PR, i propose a new function to solve this issue: #33
It basically combines reset and run functions.

This approach is going to be racy. If you run:

$ r2e run
$ r2e cleandb

you risk cleaning (and never getting mail for) any posts that go up
between ‘run’ checking a feed and ‘cleandb’ checking it again. I
think a safer approach would be to add a timestamps to the ‘seen’
entries, and then add logic to ‘run’ (or a separate ‘clean’ command)
that removes entries from ‘seen’ if they're older than a configurable
expiration duration (defaulting to 90 days?). How does that sound?

By making a contribution to this project, I certify that:

You don't have to paste the DCO text into your PR to certify it, you
need to add a Signed-off-by tag to your commit message. You can do
this for this branch with something like:

$ git checkout clean # the branch you're using for the PR
$ git commit --amend -s # add your Signed-off-by to the tip commit
$ git push --force sciunto clean # clobber your old ‘clean’ branch with the updated version

@sciunto
Copy link
Author

sciunto commented Dec 19, 2015

Yes, your proposition is better than mine. For my usecase, I was ok with the risk you mention, but your option is definitely the good one.

@wking
Copy link
Owner

wking commented Dec 19, 2015

On Fri, Dec 18, 2015 at 04:53:44PM -0800, François Boulogne wrote:

… your option is definitely the good one.

Do you have time to take a stab at it? I've been trying to catch back
up with rss2email, but the seen entries show up in a number of places
and I haven't tracked down all potentially-affected code.
Alternatively, we can wait until I have time to get a PR out, and
suggest folks use your patch in the interim if they need to trim their
cache.

@sciunto
Copy link
Author

sciunto commented Dec 19, 2015

I'm afraid I won't. My only chance is during these holidays but I already have a long todo list... :(

@wking
Copy link
Owner

wking commented Dec 20, 2015

On Sat, Dec 19, 2015 at 12:34:54PM -0800, François Boulogne wrote:

I'm afraid I won't. My only chance is during these holidays but I
already have a long todo list... :(

No worries ;). If nobody else gets to it first, I'll probably have
time before February.

@Ekleog
Copy link

Ekleog commented Apr 15, 2019

@sciunto This looks not-ready-to-be-merged, and rss2email's repository moved to https://github.com/rss2email/rss2email/ following maintainership changes, so feel free to close here and reopen your PR there if you think it should be tracked over there! :)

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.

3 participants