Skip to content

clean pending update based on Timedelta or datetime #1987

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

Closed
wants to merge 62 commits into from

Conversation

ikkemaniac
Copy link
Contributor

@ikkemaniac ikkemaniac commented Jun 9, 2020

Follow-up of #1978

Add option to pass a timedelta or datetime as var clean to .start_polling() or .start_webhook().
Updates with a timestamp before now - timedelta will be ignored.
Updates before datetime will be ignored.

I'm not familiar with the test-module to write these from scratch as this is not a 'direct' function to test. Can put in some time if y'all can point me to similar examples?

Maybe needs some cleanup but need the gh test bot results for that.

@ikkemaniac ikkemaniac changed the title Timedelta clean pending update based on Timedelta Jun 9, 2020
@Bibo-Joshi
Copy link
Member

Hi. Thanks for the PR. TBH, I haven't looked in too much detail, yet.

Regarding tests: I'm a bit surprised that we don't test clean at all. But If I see correctly, it should be enough to mock Bot.get_updates to return a custom list of updates. You can add monkeypatch to the args of the test function and use monkeypatch.setattr(bot, 'get_updates', 'some_func_to_call_instead_of_get_updates') for that. We use monkeypatch a lot in the tests, so you should be able to find examples :)

A few other things I noticed:

  • It would be desirable to also allow datetime input for clean (naive datetimes should be interpreted as UTC)
  • After Add tz kwarg to from_timestamp() #1621 message.date is already timezone aware, so you shouldn't have to replace the timezone
  • In fact, we'll need to make sure that different timezones are handled correctly, especially for datetime. We have a custom fixture timezone for that, that you can use as arg for the test functions

If you have more questions, please don't hesitate and ask :)

@ikkemaniac
Copy link
Contributor Author

Ok, I'll look into it. Is the use of **kwargs like this accepted?

@Bibo-Joshi
Copy link
Member

Ok, I'll look into it. Is the use of **kwargs like this accepted?

Mh … I guess it would be cleaner to either add an explicit argument action_db_kwargs or use functools.partial to pass the timedelta/datetime to bootstrap_clean_time…

@Bibo-Joshi Bibo-Joshi added the 📋 pending-reply work status: pending-reply label Jun 10, 2020
ikkemaniac added 2 commits June 10, 2020 22:00
…erify timezone is set, fix it if not. Throw ValueError is timedelta < 1 sec or datetime not < '1 sec' from now.

Still has a lot of debugging stuff in there.
@ikkemaniac
Copy link
Contributor Author

A few other things I noticed:

* It would be desirable to also allow `datetime` input for `clean` (naive datetimes should be interpreted as UTC)

* After #1621 `message.date` is already timezone aware, so you shouldn't have to replace the timezone

* In fact, we'll need to make sure that different timezones are handled correctly, especially for `datetime`. We have a custom fixture `timezone` for that, that you can use as arg for the test functions

Done. Freaking hate handling datetimes....

Ok, I'll look into it. Is the use of **kwargs like this accepted?

Mh … I guess it would be cleaner to either add an explicit argument action_db_kwargs or use functools.partial to pass the timedelta/datetime to bootstrap_clean_time…

Done, used functools.partial. Thx btw, learned something new today :)

@ikkemaniac
Copy link
Contributor Author

ikkemaniac commented Jun 10, 2020

note to self
TODO:

  • if code accepted copy code to start_webhook
  • Testing.py
  • cleanup

@Bibo-Joshi Bibo-Joshi added 📋 pending-review work status: pending-review and removed 📋 pending-reply work status: pending-reply labels Jun 10, 2020
@ikkemaniac
Copy link
Contributor Author

ikkemaniac commented Jun 10, 2020

I keep running into issues w/ pytest to get it running properly (keep ketting ImportWarning. Do y'all have a working docker-ptb-dev available, amd64-*nix?

@Poolitzer
Copy link
Member

Poolitzer commented Jun 11, 2020

No but we have dev requirements did you install them?

@ikkemaniac
Copy link
Contributor Author

Basically, I fear that we might construct a very specific solution for a complex use case, which might lead to it either being not very useful and/or people wanting to add more and more options to it in order to adapt to their wishes.
But maybe you see things differently and/or have a clever solution for the things a mentioned. Anyway, I'd like to hear what you think :)

I haven't been able to come up with a all encompassing solution ... i guess you are right, it's too specific and not worth the effort.

I'll close this PR.

@ikkemaniac ikkemaniac closed this Jun 26, 2020
@Bibo-Joshi
Copy link
Member

Okay. Still, thanks for taking the time and making the effort. It's always appreciated :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📋 pending-review work status: pending-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants