SOBotics / notepad

Small notepad bot for the room
0 stars 3 forks source link

Allow multiple formats for time. #6

Closed danbopes closed 6 years ago

danbopes commented 6 years ago

This PR adds the ability to have multiple datetime formats for notepad. This greatly improves the current functionality by allowing users to specify datetimes in different formats. This closes #3.

Acceptable time formats now: 2d => 2880 minutes 2d5m => 2885 minutes 2d1h5m => 2945 minutes 2h => 120 minutes 30 => 30 minutes

This remains completely backwards compatible, allowing users to not specify any time parameter (d, h, m) and defaulting to minutes.

Word of note: This is my literal first python PR. Feel free to improve/comment. I did test and make sure this was working correctly, and you can see the results in the chatroom here: https://chat.stackoverflow.com/rooms/177119/notepaddev-test

BaumMitAugen commented 6 years ago

Thank so much for the PR!

One small suggestion that seems helpful would be using datetime.timedelta instead of adding the seconds by hand. We can then extract the seconds via the total_seconds() member.

I spoke to some Python savvy guy (Andras), who gave me

In [279]: import re
    ...: from datetime import timedelta
    ...:
    ...: pattern = '(?:(?P<days>\d+)d)? \s* (?:(?P<hours>\d+)h)? \s* (?:(?P<minutes>\d+)m)? \s* (?:(?P<seconds>\d+)s)?'
    ...: string = '15d 3s'
    ...:
    ...: res = re.match(pattern, string, re.VERBOSE)
    ...: if not res:
    ...:     raise ValueError('Malformed string.')
    ...:
    ...: spec = {key:int(val) if val else 0 for key,val in res.groupdict().items()}
    ...: delta = timedelta(**spec)
    ...:
    ...: dt = delta.total_seconds()

as an example.

We could go even further if we want and leave the parsing to something like dateutil entirely, but I don't think we need that. Feel free to check it out if you have fun, though. =D

And thanks again.

danbopes commented 6 years ago

This is pretty over my head (As I know nothing about python ATM):

spec = {key:int(val) if val else 0 for key,val in res.groupdict().items()}
delta = timedelta(**spec)

You had everything formatted as minutes, so I assumed to just leave it the same. If he gave you that, why not submit it as a PR? This issue seemed backlogged for several months now, so I tried to tackle the issue. If you're more comfortable with his code, feel free to close this out and merge his in.

adeak commented 6 years ago

I don't want to hijack your PR, I just mused a bit about the code to Baum.

In case you're interested, spec is a dict comprehension that loops over key:value pairs in res.groupdict() which contains the potential matches with the keys corresponding to the name of each named group. These matches are either an int-valued string (if given) or None (if missing), so we either convert the value to int (if it's truthy, i.e. not None) or set 0 instead of a missing value. Minor note: it would probably be better to do 0 if val is None else int(val) to handle broken cases where a character is given but a number isn't, such as '15d h 3s'.

And then the dict is unpacked into keyword arguments in timedelta, i.e. it's a dynamic equivalent of passing delta = timedelta(days=number_of_days, hours=number_of_hours, minutes=number_of_minutes, seconds=number_of_seconds).

BaumMitAugen commented 6 years ago

Thank you Andras!

Yes, the dic comprehension stuff looks fairly clear to me, but I just learned about the fancy regex business. Tried to apply to our code (https://github.com/SOBotics/notepad/pull/7), what do you think?

(Re the backlog: I know, this is my fault. I'm sorry. I did not mean to hijack this, thanks again for your help.)