WIP: Add support for categories. To fix #10 #323

christf · 2018-09-17T22:19:02Z

This adds categories to todoman.

implement setting --category multiple times
implement test for multiple categories
adjust table layout
allow to write to the new table structure
allow to edit the structure. Currently "aaaaa,bbbb" will be written as "a,a,a,a,a,,,b,b,b,b" when entered in the interactive editor. However it will be fine when specifying via clia.
allow to read from the new table structure
handle the strings, forming a list for the UI.
allow to filter tasks having one category
allow to filter tasks having multiple categories

WhyNotHugo

As mentioned in #10:

Regarding the commas issue, it looks like using t.set_inline('category', ['foo','bar']) sets them properly (and also escapes commas INSIDE categories properly).

This probably needs some changes in TodoWritter.set_field (overriding the behavior for CATEGORIES).

I also think we might be able to use the multiple options feature here, so category names can actually contain commas (which would actually be escaped). This would slightly change usage to:

todo new --category foo --category bar Some description

I think this is slightly tidier than forcing people to comma-separate, what do you think about this?

Looking pretty good so far though, thanks!

tests/test_cli.py

penguineer · 2018-09-18T09:59:52Z

TodoWriter.set_field calls add on the vtodo. This is icalendar.Component.add, with the relevant part there:
https://github.com/collective/icalendar/blob/master/src/icalendar/cal.py#L186

It seems that the categories list is not recognized as such (matches the remove of ",".join(categories) in the commit, because it put a comma after each character – sign that a string was handed over.

Possible solutions:

put the comma separated list in a vText object
fix the issue that the categories field is not recognized as list. Fixing the join issue could be a milestone here.

penguineer · 2018-09-18T19:05:46Z

I've just had a look into the proposed implementation and I vote for removing the lv:easy tag from #10. While it may seem easy to implement categories similar to location, there is the major difference that each TOOD has only one location with no further format, while there can be multiple categories.

We can take the easy route and store the categories comma-separated in a database text field. This would mostly mean:

put categories together when the parameter is repeated
initialize the Todo.categorie with None instead of []
However, with this implementation it is hard to filter by a specific category, which is the ultimate goal.

Proper database design would suggest that there is a table with tuples (category, UID), where proper clean-up, aka remove all UID entries from this mapping when a UID is removed from the cache, is necessary.

Unless we go with the less complex, but also less efficient approach:

Use a text field to store a comma-separated list of categories
Filter categories by loading all Todos with a category and then check for each item if it matches the filter.
I guess the only necessary fix for this solution is to convert the category string to vText before adding the field to the icalendar.Component.

christf · 2018-09-19T18:11:28Z

ok, let's just lay out the tasks then.

…shown

christf · 2018-09-19T23:22:34Z

unfortunately the model ties the name of the command line options and the name of the field in the ics file. So the option will be called --categories.

WhyNotHugo

This is looking pretty good so far, thanks for the big effort (this is actually a lot less simple than I'd anticipated!).

I've mostly minor comments, and nothing critical. Thanks!

todoman/cli.py

WhyNotHugo · 2018-09-24T15:49:18Z

todoman/formatters.py

+        if categories is None or categories == '':
+            return None
+        else:
+            return categories


This should actually be return c for c in caterories.split(','), so that this returns a list of categories.

WhyNotHugo · 2018-09-24T15:49:46Z

todoman/cli.py

@@ -355,6 +370,9 @@ def new(ctx, summary, list, todo_properties, read_description, interactive):

    for key, value in todo_properties.items():
        if value:
+            logger.debug("property: " + key + " value: " + ','.join(value) +  " (" + str(type(value)) + ")" )
+            if key == "categories":
+                value = [v for v in value]


I believe this should actually be done in parse_category (see my other comment below).

todoman/cli.py

WhyNotHugo · 2018-09-24T15:52:27Z

todoman/interactive.py

@@ -159,6 +164,7 @@ def _save_inner(self):
        self.todo.summary = self.summary
        self.todo.description = self.description
        self.todo.location = self.location
+        self.todo.categories = self.categories.split(',')


I believe we might also want to strip any whitespace in each category, so that Offline, Work ends up as Office and Work, rather than Office and Work. (note the extra space in the last one).

Maybe:

self.todo.categories = [c.strip() for c in self.categories.split(',')]

todoman/model.py

WhyNotHugo · 2018-09-24T15:55:57Z

todoman/model.py

-            self.vtodo.add(name, value)
+            if name == 'categories':
+                v = icalendar.prop.vInline(','.join(value))
+                self.vtodo.add(name, v)


I think using set_inline here actually sets the category property escaping commas as expected.

That means you also don't have to split when reading back from the cache.

todoman/model.py

WhyNotHugo · 2018-09-24T16:04:21Z

Regarding this comment:

unfortunately the model ties the name of the command line options and the name of the field in the ics file. So the option will be called --categories.

I believe it's possible to make the parameter name and flag name different, see here.

I know this documentation change is relatively recent, but I believe click>=6 has the same behaviour (as seen in 9b111c5)

…nstead of category

WhyNotHugo

Mostly looking pretty good. Let me know if you need any help with anything of have any doubts and I can take a closer look.

Thanks!

WhyNotHugo · 2018-09-28T01:56:28Z

todoman/model.py

-            self.vtodo.add(name, value)
+            if name == 'categories':
+#                v = icalendar.prop.vInline(','.join(value))
+                v = self.vtodo.set_inline(name, value)


Looks like this needs to be if value is not None.

set_inline in the icalender library is defined here:
https://github.com/collective/icalendar/blob/master/src/icalendar/cal.py#L256

WhyNotHugo · 2018-10-18T04:45:52Z

icalendar had some API changes around the whole categories part, so this PR might need some updating.
I've also had to update master to deal with those upstream changes, so there'll be new conflicts.

penguineer · 2018-10-18T08:56:31Z

icalendar had some API changes around the whole categories part, so this PR might need some updating.

Shouldn't todoman specify the icalendar version in the requirements?

WhyNotHugo · 2018-10-18T18:43:42Z

The latest version specifies a minimum version, due to this API change being backwards incompatible.

WhyNotHugo requested changes Sep 18, 2018

View reviewed changes

tests/test_cli.py Show resolved Hide resolved

christf force-pushed the category branch 2 times, most recently from 0ff3d90 to 95ff7e6 Compare September 19, 2018 23:02

christf added 2 commits September 20, 2018 01:13

Add support for categories. This fixes pimutils#10

2a47506

cli.py: fix typo: incomplete tasks are show. => incomplete tasks are …

3a0f79e

…shown

christf force-pushed the category branch from 95ff7e6 to 3a0f79e Compare September 19, 2018 23:14

christf changed the title ~~Add support for categories. To fix #10~~ WIP: Add support for categories. To fix #10 Sep 19, 2018

christf added 2 commits September 20, 2018 02:20

fixup! Add support for categories. This fixes pimutils#10

65711f5

fixup! Add support for categories. This fixes pimutils#10

e9fa829

WhyNotHugo reviewed Sep 24, 2018

View reviewed changes

just sharing the latest code - still not working yet- () is written i…

107357a

…nstead of category

WhyNotHugo reviewed Sep 28, 2018

View reviewed changes

WhyNotHugo mentioned this pull request Oct 18, 2018

Fix crash due to changes in icalendar 4.0.3 #328

Merged

penguineer mentioned this pull request Oct 19, 2018

Small fixes #329

Merged

Base automatically changed from master to main March 9, 2021 18:59

0styx0 mentioned this pull request Feb 2, 2022

feat: Implement category support #475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add support for categories. To fix #10 #323

WIP: Add support for categories. To fix #10 #323

christf commented Sep 17, 2018 •

edited

Loading

WhyNotHugo left a comment

penguineer commented Sep 18, 2018

penguineer commented Sep 18, 2018

christf commented Sep 19, 2018 •

edited

Loading

christf commented Sep 19, 2018

WhyNotHugo left a comment

WhyNotHugo Sep 24, 2018

WhyNotHugo Sep 24, 2018

WhyNotHugo Sep 24, 2018

WhyNotHugo Sep 24, 2018

WhyNotHugo commented Sep 24, 2018

WhyNotHugo left a comment

WhyNotHugo Sep 28, 2018

penguineer Sep 28, 2018

WhyNotHugo commented Oct 18, 2018

penguineer commented Oct 18, 2018

WhyNotHugo commented Oct 18, 2018

WIP: Add support for categories. To fix #10 #323

Are you sure you want to change the base?

WIP: Add support for categories. To fix #10 #323

Conversation

christf commented Sep 17, 2018 • edited Loading

WhyNotHugo left a comment

Choose a reason for hiding this comment

penguineer commented Sep 18, 2018

penguineer commented Sep 18, 2018

christf commented Sep 19, 2018 • edited Loading

christf commented Sep 19, 2018

WhyNotHugo left a comment

Choose a reason for hiding this comment

WhyNotHugo Sep 24, 2018

Choose a reason for hiding this comment

WhyNotHugo Sep 24, 2018

Choose a reason for hiding this comment

WhyNotHugo Sep 24, 2018

Choose a reason for hiding this comment

WhyNotHugo Sep 24, 2018

Choose a reason for hiding this comment

WhyNotHugo commented Sep 24, 2018

WhyNotHugo left a comment

Choose a reason for hiding this comment

WhyNotHugo Sep 28, 2018

Choose a reason for hiding this comment

penguineer Sep 28, 2018

Choose a reason for hiding this comment

WhyNotHugo commented Oct 18, 2018

penguineer commented Oct 18, 2018

WhyNotHugo commented Oct 18, 2018

christf commented Sep 17, 2018 •

edited

Loading

christf commented Sep 19, 2018 •

edited

Loading