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 1 commit
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
Prev Previous commit
Next Next commit
until/every empty string should be mapped to None
Fix #999.

This also addresses a defect where editing a previously saved event
cause the 3 rrule properties (freq, until and interval) to have
incorrect default type (list instead of str), and freq value is forced
to lower case as the current code requires it but more important that is
how the values are presented in the interactive session.
  • Loading branch information
Allan Wind committed May 28, 2021
commit 09c0ccb2387d9d66ed5ff4bbb0f016aee7bcd5bb
44 changes: 26 additions & 18 deletions khal/controllers.py
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ 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'] = ''

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,24 +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 ""
interval = recur["interval"] if "interval" in recur else ""
if not freq:
freq = 'None'
freq = prompt('frequency (or "None")', freq)
if freq == 'None':
event.update_rrule(None)
else:
until = prompt('until (or "None")', until)
if until == 'None':
until = None
interval = prompt('every (or "None")', interval)
if interval == 'None':
interval = 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)
event.update_rrule(rrule)
else:
rrule = None
event.update_rrule(rrule)
edited = True
elif choice == "alarm":
default_alarms = []
Expand Down
4 changes: 2 additions & 2 deletions khal/parse_datetime.py
Original file line number Diff line number Diff line change
Expand Up @@ -423,8 +423,8 @@ def rrulefstr(repeat, until, every, locale):
try:
int(every)
except ValueError:
logger.fatal("Invalid value for every option." +
"Must be an integer")
logger.fatal("Invalid value for every option. " +
"Must be an integer")
raise FatalError()
rrule_settings['interval'] = every
return rrule_settings
Expand Down