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

Implement the remind command #68

Merged
merged 1 commit into from
Jan 31, 2017
Merged

Implement the remind command #68

merged 1 commit into from
Jan 31, 2017

Conversation

WhyNotHugo
Copy link
Member

@WhyNotHugo WhyNotHugo commented Jan 12, 2017

The main idea behind this is to allow external scripts to remind of soon-to-expire tasks, or just allow the user to check if they're up to date on tasks or now.

Some stuff that needs to be defined:

  • Do we want due_in to be a flag or configuration setting? I'm inclined to make it the latter, though both might be possible.
  • Do we want overdue to be a flag or configuration setting? Same doubt here.

Pending:

  • Tests:
    • Include ones with tz-aware tasks, and tz-naive tasks.
  • A --porcelain flag, to keep the format permanently frozen so scripts can parse this easily.

I think it's safe to say that this would close #67

Please, chime in on the functionality (I'll continue polishing the code in the meantime, it's a mere prototype)

@untitaker
Copy link
Member

untitaker commented Jan 14, 2017 via email

@WhyNotHugo
Copy link
Member Author

Good call. It's also a lot less code than I'd expect. It's a shame that the filtering for loop keeps getting uglier, but I have come plans in mind for that one. ^^

def list(ctx, lists, all, urgent, location, category, grep, sort, reverse):
@click.option('--due', default=None, help='Only show tasks due in DUE hours',
type=int)
# TODO: we might want a `porcelain` flag here to print this is a
Copy link
Member

Choose a reason for hiding this comment

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

JSON seems like the obvious candidate. There are CLI tools dedicated to selecting fields from JSON, for scripting purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking something a bit simpler, like this (due date, percent, title). Basically inspired in git status --porcelain:

2017-01-01 0 Do stuff
2017-01-01 100 Do more stuff


JSON is nicer for apps to read, but a bit more complex for bash scripts. I guess it depends a bit on who we're targeting with this, but I'd much rather stick to the simplest-possible format.

Copy link
Member

@untitaker untitaker Jan 23, 2017

Choose a reason for hiding this comment

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

For scripting purposes https://github.com/stedolan/jq can be used, docs at https://stedolan.github.io/jq/

Copy link
Member Author

Choose a reason for hiding this comment

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

You've convinced me; +1 for JSON.

@@ -326,11 +338,23 @@ def list(ctx, lists, all, urgent, location, category, grep, sort, reverse):
(not pattern or (
pattern.search(todo.summary) or
pattern.search(todo.description)
))
)) and
(
Copy link
Member

Choose a reason for hiding this comment

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

On all other lines the following style is used:

(a
 b)

but here you use:

(
  a
  b
)

(todo.due <= max_naive)) or
(todo.due and
todo.due.tzinfo and
(todo.due <= max_aware)))
Copy link
Member

Choose a reason for hiding this comment

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

This is getting out of hand, I think it's time for a new function

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I kind of makes me lose faith a bit and just work on #23.

Copy link
Member

Choose a reason for hiding this comment

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

Moving this into a function is really easy compared to implementing a new cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC, python does some magic optimizations for this sort of list comprehension, I'd have to see if they still apply here, and if a function has any drawbacks (I admit I'm a bit ignorant on this, and might be mistaken).

Copy link
Member

Choose a reason for hiding this comment

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

I'd argue that if you care about this, you'd have to rewrite it in a compiled language.

@WhyNotHugo WhyNotHugo changed the title Implement WIP: the remind command Implement the remind command Jan 31, 2017
@WhyNotHugo WhyNotHugo merged commit 178003d into master Jan 31, 2017
@WhyNotHugo WhyNotHugo deleted the remind branch January 31, 2017 13:49
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.

List by "today", "this week"
2 participants