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

remind: add support for date in .at command #1590

Merged
merged 3 commits into from
Jul 18, 2019

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented May 8, 2019

This PR is based on #1581, and it replaces PR #1162.
Also fix #1148.

It can replace PR #1581 if someone is brave enough to do the review of all the changes in one go.
I added an extensive test-suite to make sure the remind .at command works as intended.

I made .at opinionated about how date are expressed, with these choices:

  • YYYY-mm-dd is valid, as dd-mm-YYYY
  • dd-mm-YY is also valid
  • screw mm-dd-YY (or mm-dd-YYYY) format
  • dd-mm is valid too, as mm-YYYY and YYYY-mm
  • but there is no such thing as mm-YY or YY-mm
  • date separator can be either -, ., or /, and it must be the same between each parts

Even with all the unit-tests I added, I spent a good one hour with a live Sopel instance, and everything went according to the specs.

@Exirel Exirel added the Feature label May 8, 2019
@Exirel Exirel added this to the 7.0.0 milestone May 8, 2019
@Exirel Exirel requested review from dgw and a team May 8, 2019 10:49
@Exirel
Copy link
Contributor Author

Exirel commented May 8, 2019

Wouhouh, tests pass with Python 2.7! That was my main concern at this point, given the extensive test suite I ran with Python 3.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Some little things from going through the single commit unique to this (not pulled from the base branch in #1581). Very few little things, in fact, but an important class-design consideration did come up.

sopel/modules/remind.py Outdated Show resolved Hide resolved
sopel/modules/remind.py Outdated Show resolved Hide resolved
test/modules/test_modules_remind.py Outdated Show resolved Hide resolved
sopel/modules/remind.py Outdated Show resolved Hide resolved
@Exirel Exirel force-pushed the remind-at-future branch from bdf1a34 to 601dcb1 Compare May 11, 2019 14:51
@dgw dgw force-pushed the remind-at-future branch from 3aeb58e to 5b393e4 Compare May 12, 2019 03:01
@dgw
Copy link
Member

dgw commented May 12, 2019

Would like to merge this hot on the heels of #1581, so I rebased it myself to get it ready for review sooner. @Exirel if you would rather I didn't do such things in the future on your PRs, just say so. :)

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I wanted to make this just a "Comment" review, but there actually are enough things I would like to change that it warrants making that official. It's really close, though! I think we can merge this tomorrow. 🤞

sopel/modules/remind.py Show resolved Hide resolved
sopel/modules/remind.py Outdated Show resolved Hide resolved
sopel/modules/remind.py Outdated Show resolved Hide resolved
sopel/modules/remind.py Outdated Show resolved Hide resolved
@Exirel Exirel force-pushed the remind-at-future branch from 195b6f4 to 911f1e8 Compare May 12, 2019 10:29
@Exirel
Copy link
Contributor Author

Exirel commented May 12, 2019

Uh oh... apparently I made a mistake: since you forced-push to that branch, I had merge conflict locally for some unknown reason, and I think I messed-up the squashing... :-/

@Exirel
Copy link
Contributor Author

Exirel commented Jun 29, 2019

After double check, I believe it's all good. Ready for a new (and hopefully last) round of review before merge.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

This looks good now, and I hope it's not because I'm tired. 😹

Just one line note that can be addressed if @Exirel has time to do so before I merge this and agrees that it's worth doing.

sopel/modules/remind.py Show resolved Hide resolved
Exirel and others added 3 commits July 5, 2019 15:01
With corrections from dgw

Co-Authored-By: dgw <dgw@technobabbl.es>
Co-Authored-By: dgw <dgw@technobabbl.es>
@Exirel Exirel force-pushed the remind-at-future branch from c9832a3 to ddf52fe Compare July 5, 2019 13:16
@Exirel
Copy link
Contributor Author

Exirel commented Jul 5, 2019

@dgw done

@Exirel Exirel removed the request for review from a team July 5, 2019 13:29
@dgw dgw merged commit e3a8a47 into sopel-irc:master Jul 18, 2019
@Exirel Exirel deleted the remind-at-future branch September 5, 2019 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[remind] .at does not accept a future date, limiting reminders to next 24 hours
2 participants