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

WIP: Use parsedatetime for khal list #491

Closed
wants to merge 1 commit into from

Conversation

untitaker
Copy link
Member

No description provided.

@untitaker
Copy link
Member Author

Example usage:

khal list -e "one week"  # list seven days from one
khal list -s "tomorrow"  # list events tomorrow
khal list -s "tomorrow" -e "one week"  # list events from tomorrow to (tomorrow + 7 days = 8 days)

@untitaker untitaker mentioned this pull request Aug 29, 2016
@geier
Copy link
Member

geier commented Aug 29, 2016

Some first ideas, opinions, reservations:

  • well tested code base (much better than our own)
  • khal new and at should also use parsedatetime
  • would probably make configuring of all [long](date|time)formats at the very
    least optionally (which is probably a good thing for most users)
  • I dislike the khal list -s 2017-4-4 -e 2017-4-12 format and much prefer
    khal list 2017-4-4 - 2017-4-4 (which event works out of the box with
    evalRanges), but I'm not against making -s/-e optional
  • as a - would be needed between start and end date (probably a good idea
    anyway), we should figure out a way to warn users for at least on release who
    enter start and end date without a -
  • if setting a locale which isn't en, daynames, relative descriptions etc get
    translated as well (which is a big minus in my book)
  • users can only set locale, no finer grained control over datetime in/output
    (but probably not necessary)
  • with german locale settings cal.parse('1.10.') works, but
    cal.evalRanges('1.10.-10.10.') does not, strange

The biggest plus for parsedatetime is, that it probably has fewer bugs than our own implemenation(s).

@youngmoney
Copy link
Contributor

I believe that at, new, and list all use the same function to parse dates and more (at least I think I wrote it that way) if true then this is where the parse date time should go

Just for some background, gcalcli allows optional parse datetime, if it is installed it will use it, otherwise it uses its own stricter parser (like ours) I say we try something like that, maybe even only use parse datetime if our parser fails and it is installed

Anyway just my opinion

On Aug 29, 2016, at 10:20 AM, Christian Geier notifications@github.com wrote:

Some ideas, opinions, reservations:

well tested code base (much better than our own)
khal new and at should also use parsedatetime
would probably make configuring of all longformats at the very least optionally (which is probably a good thing for most users)
I dislike the khal list -s 2017-4-4 -e 2017-4-12 format and much prefer khal list 2017-4-4 - 2017-4-4 (which event works out of the box with evalRanges), but I'm not against making -s/-e optional
as a - would be needed between start and end date (probably a good idea anyway), we should figure out a way to warn users for at least on release who enter start and end date without a -
if setting a locale which isn't en, daynames, relative descriptions etc get translated as well (which is a big minus in my book)
users can only set locale, no finer grained control over datetime in/output (but probably not necessary)
with german locale settings cal.parse('1.10.') works, but cal.evalRanges('1.10.-10.10.') does not, strange

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@untitaker
Copy link
Member Author

khal new and at should also use parsedatetime

I'd rather remove at in favor of this command (entering just the start date gets you the same behavior).

would probably make configuring of all longformats at the very least optionally (which is probably a good thing for most users)

In todoman we fall back to the configured dateformat, also an option here.

I dislike the khal list -s 2017-4-4 -e 2017-4-12 format and much prefer khal list 2017-4-4 - 2017-4-4

Strong disagree, makes the whole thing much more complex. Arguments are not optional. Options are optional.

if setting a locale which isn't en, daynames, relative descriptions etc get translated as well

I didn't observe that behavior at all. If I set LANG=de_DE.UTF-8, weeknames in ikhal are translated, but parsedatetime behaves the same for the values "in one week", "thuesday", etc.

@geier
Copy link
Member

geier commented Aug 29, 2016

re locale: you are right, setting a environment variable works fine, I followed the example in the documentation and than it doesn't.

@geier
Copy link
Member

geier commented Sep 6, 2016

I won't merge anything that removes the argument parsing, I'm not against having options as an alternative.

@untitaker
Copy link
Member Author

I don't understand why you insist on having your own argument parsing instead of using normal options. This makes the entire CLI harder to understand since it's a non-standard thing.

@geier
Copy link
Member

geier commented Sep 7, 2016

Quoting Markus Unterwaditzer (2016-09-06 22:31:48)

I don't understand why you insist on having your own argument parsing instead of using normal options.

I do not insist on having my own argument parsing, I'll happily use any module that does this for us. Also, I do reject the notion that options are normal and arguments are not, e.g., see the classic UNIX utility cal(1).

So, why do I insist on it:

  1. I'm lazy, typing khal list 2016-10-1 2016-10-20 is easier than typing khal list -s 2016-10-1 -e 2016-10-20, even worse when datetimes are given and need to be put into quotation marks.
  2. I strongly believe, that computer programs should strive to be able to understand unstructured user input (as long as its unambiguous). Of course, khal's commands currently do a very bad job at this, but options-only would be a step into the wrong direction in my opinion. See Use more convenient dates and times #169 and Feature request: make at command more intelligent #332 for some examples of what khal should be able to understand. I'd also like to be able to just pipe an email containing an event description into khal which should extract the relevant information itself. Yes, this shifts complexity into the program, but I for programs I spend more time using than building, that's fine.

This makes the entire CLI harder to understand since it's a non-standard thing.

Regarding the claim of that being non-standard, see cal(1) (by the way, the very first script called khal was a replacement for cal, because I disliked both cal's and ncal's output). But, as said before, I'm happy to have khal's commands either accept both, arguments and options, or have two sets of commands, one that accepts only options and one that accepts arguments.

Quick anecdote only somewhat connected to any of this:

Just today, a colleague of mine looked at scipy's current documentation for its Mann-Whitney-U Test (function signature: mannwhitneyu(x, y, use_continuity=True, alternative=None), where alternative can be one of None, ‘less’, ‘two-sided’, or ‘greater’). The colleague than went ahead and used it (wrongly) like this: mannwhitneyu(x, y, 'two-sided'), which produced a result, but was obviously not what the colleague wanted, who expected the function to sort out what was meant. While this is mostly a usage error (which can also be somewhat attributed to python's dynamic typing), to any human it is pretty clear what my colleague wanted to achieve, and the function could have figured out what the colleague wanted to achieve.
OK, I'm rambling now...

@untitaker
Copy link
Member Author

untitaker commented Sep 7, 2016

Arguments should be used for (1..n) parameters of the same type, options for optional parameters of different types. To see what I mean, look at

  • ls, cat -- arguments are one or more files/directories. Options are there to modify the output format.
  • bash -- arguments are one or more files. Options are there to change error behavior or verbosity.
  • find -- arguments are one or more files. Options, while unnecessarily deviating from the norm (-name instead of --name) are there to filter out names or execute commands for each line.
  • diff -- arguments are, again, files. Options are there to fine-tune behavior.
  • anything else

Cal is different since it takes a day, month, or year as argument, but at least one could argue that it takes a single "date" arg, with varying precision.

Quick anecdote only somewhat connected to any of this:

I'm going to interpret this as an argument for using keyword arguments everywhere. Coincidentally options are close to that :P


I have an alternative idea:

  • Rename -s to --from
  • Make -e an argument
    khal list --from tomorrow two weeks

    khal list --from "in two weeks" two weeks

@geier geier closed this Feb 14, 2017
@geier geier removed the in progress label Feb 14, 2017
@untitaker
Copy link
Member Author

I guess it wasn't important to me after all. Oh well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants