Skip to content

Add --every / -e option to the new command #1052

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 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,4 @@ Yorick Barbanneau - git [at] epha [dot] se - https://xieme-art.org
Florian Lassenay - pizzacoca [at] aquilenet [dot] fr
Simon Crespeau - simon [at] art-e-toile [dot] com - https://www.art-e-toile.com
Fred Thomsen - me [at] fredthomsen [dot] net - http://fredthomsen.net
Allan Wind - [email protected]
1 change: 1 addition & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ not released

* DROPPED support for Python 3.5
* CHANGE: ikhal: tab (and shift tab) jump from the events back to the calendar
* NEW Add --every / -e to support INTERVAL on the command line

0.10.3
======
Expand Down
4 changes: 4 additions & 0 deletions doc/source/usage.rst
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,10 @@ Options

* **-u, --until=UNTIL** specify until when a recurring event should run

* **-e, --every=EVERY** specify at what interval a recurring event
should run. For instance, if RRULE is weekly and EVERY is 2, it
would schedule the event every 2 weeks.

* **--url** specify the URL element of the event

* **--alarms DURATION,...** will add alarm times as DELTAs comma separated for this event,
Expand Down
6 changes: 5 additions & 1 deletion khal/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,9 @@ def klist(ctx, include_calendar, exclude_calendar,
help=('Repeat event: daily, weekly, monthly or yearly.'))
@click.option('--until', '-u',
help=('Stop an event repeating on this date.'))
@click.option('--every', '-e',
help=('Frequency for repeating events.'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Referring to this as "frequency" is likely to cause confusion as that's used in the code and the RFC for "weekly"/etc (FREQ)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I agree, tried a few things different variations to make precise and concise and mixed it up in the final edit. The wording from the spec would be "positive integer representing at which intervals the recurrence rule repeats". "interval of repeating event"?

Copy link
Author

@ghost ghost May 27, 2021

Choose a reason for hiding this comment

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

I initially looked at adding/updating tests but found some that were relevant were marked xfail so I was not sure how to proceed.

type=click.INT)
@click.option('--format', '-f',
help=('The format to print the event.'))
@click.option('--alarms', '-m',
Expand All @@ -349,7 +352,7 @@ def klist(ctx, include_calendar, exclude_calendar,
@click.argument('info', metavar='[START [END | DELTA] [TIMEZONE] [SUMMARY] [:: DESCRIPTION]]',
nargs=-1)
@click.pass_context
def new(ctx, calendar, info, location, categories, repeat, until, alarms, url, format,
def new(ctx, calendar, info, location, categories, repeat, until, every, alarms, url, format,
interactive):
'''Create a new event from arguments.

Expand Down Expand Up @@ -395,6 +398,7 @@ def new(ctx, calendar, info, location, categories, repeat, until, alarms, url, f
repeat=repeat,
env={"calendars": ctx.obj['conf']['calendars']},
until=until,
every=every,
alarms=alarms,
url=url,
format=format,
Expand Down
52 changes: 32 additions & 20 deletions khal/controllers.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ def khal_list(collection, daterange=None, conf=None, agenda_format=None,


def new_interactive(collection, calendar_name, conf, info, location=None,
categories=None, repeat=None, until=None, alarms=None,
categories=None, repeat=None, until=None, every=None, alarms=None,
format=None, env=None, url=None):
try:
info = parse_datetime.eventinfofstr(
Expand Down Expand Up @@ -326,14 +326,14 @@ def new_interactive(collection, calendar_name, conf, info, location=None,
except pytz.UnknownTimeZoneError:
echo("unknown timezone")

info['description'] = prompt("description (or 'None')", default=info.get('description'))
info['description'] = prompt('description (or "None")', default=info.get('description'))
if info['description'] == 'None':
info['description'] = ''

event = new_from_args(
collection, calendar_name, conf, format=format, env=env,
location=location, categories=categories,
repeat=repeat, until=until, alarms=alarms, url=url,
repeat=repeat, until=until, every=every, alarms=alarms, url=url,
**info)

echo("event saved")
Expand All @@ -343,7 +343,7 @@ def new_interactive(collection, calendar_name, conf, info, location=None,


def new_from_string(collection, calendar_name, conf, info, location=None,
categories=None, repeat=None, until=None, alarms=None,
categories=None, repeat=None, until=None, every=None, alarms=None,
url=None, format=None, env=None):
"""construct a new event from a string and add it"""
info = parse_datetime.eventinfofstr(
Expand All @@ -355,21 +355,21 @@ def new_from_string(collection, calendar_name, conf, info, location=None,
new_from_args(
collection, calendar_name, conf, format=format, env=env,
location=location, categories=categories, repeat=repeat,
until=until, alarms=alarms, url=url, **info
until=until, every=every, alarms=alarms, url=url, **info
)


def new_from_args(collection, calendar_name, conf, dtstart=None, dtend=None,
summary=None, description=None, allday=None, location=None,
categories=None, repeat=None, until=None, alarms=None,
categories=None, repeat=None, until=None, every=None, alarms=None,
timezone=None, url=None, format=None, env=None):
"""Create a new event from arguments and add to vdirs"""
if isinstance(categories, str):
categories = [category.strip() for category in categories.split(',')]
try:
event = new_vevent(
locale=conf['locale'], location=location, categories=categories,
repeat=repeat, until=until, alarms=alarms, dtstart=dtstart,
repeat=repeat, until=until, every=every, alarms=alarms, dtstart=dtstart,
dtend=dtend, summary=summary, description=description, timezone=timezone, url=url,
)
except ValueError as error:
Expand Down Expand Up @@ -413,6 +413,24 @@ def present_options(options, prefix="", sep=" ", width=70):
return None


def prompt_none(text, default=None, lower=False, type=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This "helper" feels like it's overcomplicating things to me. Especially with the two optional arguments that are only used once each - i.e. it's called 3 times, with 2 special cases.

Copy link
Author

Choose a reason for hiding this comment

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

I added the helper to eliminate the duplicate logic for the 3 attributes. This shrunk the code from 14 to 7 lines not counting the helper. Planning on using it to fix #1054 which would make it the 4th use, and it's duplicated against for the "else" attribute case and alarm (6 uses).

No love lost on the lower case conversion special case either, but it needs to happen after the type coercing from list to str. The better alternative is probably to split the function into two: 1. convert value, if list, to str. Any idea of name for this function? 2. "None" business then handle the case conversion for frequency inline.

In either case, I was looking for input on #1050 to see if we can eliminate the magic "None" value, so this just leaves the type conversion.

# icalendar.prop.vRecur returns list values for frequency, until and
# interval with one element. However, when creating an event with
# new -i they are str values.
if isinstance(default, list):
default = default[0]

# icalendar.prop.vRecur return the freq value in upper case,
# however we only accept lower case values. For consistency
# with the interactive mode conver the value for lowercase
# for display.
if isinstance(default, str) and lower:
default = default.lower()

v = prompt(text + ' (or "None")', default=default or "None", type=type)
return v if v != "None" else None


def edit_event(event, collection, locale, allow_quit=False, width=80):
options = OrderedDict()
if allow_quit:
Expand Down Expand Up @@ -458,20 +476,14 @@ def edit_event(event, collection, locale, allow_quit=False, width=80):
except: # noqa
echo("error parsing range")
elif choice == "repeat":
recur = event.recurobject
freq = recur["freq"] if "freq" in recur else ""
until = recur["until"] if "until" in recur else ""
if not freq:
freq = 'None'
freq = prompt('frequency (or "None")', freq)
if freq == 'None':
event.update_rrule(None)
freq = prompt_none("frequency", event.recurobject.get("freq"), lower=True)
if freq:
until = prompt_none("until", event.recurobject.get("until"))
interval = prompt_none("interval", event.recurobject.get("interval"), int)
rrule = parse_datetime.rrulefstr(freq, until, interval, locale)
else:
until = prompt('until (or "None")', until)
if until == 'None':
until = None
rrule = parse_datetime.rrulefstr(freq, until, locale)
event.update_rrule(rrule)
rrule = None
event.update_rrule(rrule)
edited = True
elif choice == "alarm":
default_alarms = []
Expand Down
4 changes: 2 additions & 2 deletions khal/icalendar.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def split_ics(ics, random_uid=False, default_timezone=None):

def new_event(locale, dtstart=None, dtend=None, summary=None, timezone=None,
allday=False, description=None, location=None, categories=None,
repeat=None, until=None, alarms=None, url=None):
repeat=None, until=None, every=None, alarms=None, url=None):
"""create a new event

:param dtstart: starttime of that event
Expand Down Expand Up @@ -121,7 +121,7 @@ def new_event(locale, dtstart=None, dtend=None, summary=None, timezone=None,
if url:
event.add('url', icalendar.vUri(url))
if repeat and repeat != "none":
rrule = rrulefstr(repeat, until, locale)
rrule = rrulefstr(repeat, until, every, locale)
event.add('rrule', rrule)
if alarms:
for alarm in alarms.split(","):
Expand Down
10 changes: 9 additions & 1 deletion khal/parse_datetime.py
Original file line number Diff line number Diff line change
Expand Up @@ -413,12 +413,20 @@ def guessrangefstr(daterange, locale,
)


def rrulefstr(repeat, until, locale):
def rrulefstr(repeat, until, every, locale):
if repeat in ["daily", "weekly", "monthly", "yearly"]:
rrule_settings = {'freq': repeat}
if until:
until_dt, is_date = guessdatetimefstr(until.split(' '), locale)
rrule_settings['until'] = until_dt
if every:
try:
int(every)
except ValueError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be clearer here (and more efficient, not that it really matters) to use if every.isdigit() here, as you're not using the cast.

Copy link
Author

@ghost ghost May 27, 2021

Choose a reason for hiding this comment

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

Yes, later discovered that prompt had a type feature to constraint the input which is a better way of handling it, and later learned that it also have to be positive to I will have a change on that coming.

Copy link
Collaborator

Choose a reason for hiding this comment

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

fwiw .isdigit() would return false for negatives :)

Copy link
Author

Choose a reason for hiding this comment

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

isdigit() ftw :-)

Copy link
Author

@ghost ghost May 28, 2021

Choose a reason for hiding this comment

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

Well, turns out that stores values are type vInt so cannot use isdigit() on that, and I need to check for int(every) > 0. Just a heads up if you wonder why I didn't end up usign isdigit() after all.

logger.fatal("Invalid value for every option. " +
"Must be an integer")
raise FatalError()
rrule_settings['interval'] = every
return rrule_settings
else:
logger.fatal("Invalid value for the repeat option. \
Expand Down
1 change: 1 addition & 0 deletions tests/cli_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,7 @@ def test_new_interactive_extensive(runner):
'l\non a boat\n'
'p\nweekly\n'
'1.1.2018\n'
'1\n'
'a\n30m\n'
'c\nwork\n'
'n\n'
Expand Down