Make Entry.important and set_entry_important(important=) optional bools.

Also, set_entry_read/important() should take exactly True, False, None
(instead of anything you can call bool() on).

Also, update the mark_as_read plugin to explicitly set important=False,
and read_modified and important_modified None.

For #254.

src/reader/_storage.py

@@ -731,7 +731,7 @@ def mark_as_important(
         self,
         feed_url: str,
         entry_id: str,
-        important: bool,
+        important: bool | None,
         modified: datetime | None,
     ) -> None:
         with self.get_db() as db:
@@ -940,11 +940,11 @@ def _add_or_update_entries(
                     FROM entries
                     WHERE id = :id AND feed = :feed_url
                 ),
-                coalesce((
+                (
                     SELECT important
                     FROM entries
                     WHERE id = :id AND feed = :feed_url
-                ), 0),
+                ),
                 (
                     SELECT important_modified
                     FROM entries
@@ -1404,7 +1404,7 @@ def entry_factory(t: tuple[Any, ...]) -> Entry:
         tuple(Enclosure(**d) for d in json.loads(t[20])) if t[20] else (),
         t[21] == 1,
         t[22],
-        t[23] == 1,
+        t[23] == 1 if t[23] is not None else None,
         t[24],
         t[25],
         t[26],
@@ -1434,7 +1434,9 @@ def apply_entry_filter_options(
         add(f"{'' if read else 'NOT'} entries.read")
 
     if important is not None:
-        add(f"{'' if important else 'NOT'} entries.important")
+        add(
+            f"{'' if important else 'NOT'} (entries.important IS NOT NULL AND entries.important)"
+        )
 
     if has_enclosures is not None:
         add(

src/reader/core.py

@@ -1311,7 +1311,7 @@ def set_entry_read(
 
         Args:
             entry (tuple(str, str) or Entry): (feed URL, entry id) tuple.
-            read (bool): Mark the entry as read if true (default),
+            read (bool): Mark the entry as read if true,
                 and as unread otherwise.
             modified (datetime or None):
                 Set :attr:`~Entry.read_modified` to this.
@@ -1329,6 +1329,9 @@ def set_entry_read(
             The ``entry`` and ``read`` arguments are now positional-only.
 
         """
+        if read not in (True, False):
+            raise ValueError("read should be one of (True, False)")
+
         modified_naive: datetime | None
         if isinstance(modified, MissingType):
             modified_naive = self._now()
@@ -1338,7 +1341,7 @@ def set_entry_read(
             modified_naive = modified.astimezone(timezone.utc).replace(tzinfo=None)
 
         feed_url, entry_id = _entry_argument(entry)
-        self._storage.mark_as_read(feed_url, entry_id, bool(read), modified_naive)
+        self._storage.mark_as_read(feed_url, entry_id, read, modified_naive)
 
     def mark_entry_as_read(self, entry: EntryInput, /) -> None:
         """Mark an entry as read.
@@ -1385,7 +1388,7 @@ def mark_entry_as_unread(self, entry: EntryInput, /) -> None:
     def set_entry_important(
         self,
         entry: EntryInput,
-        important: bool,
+        important: bool | None,
         /,
         modified: MissingType | None | datetime = MISSING,
     ) -> None:
@@ -1394,8 +1397,8 @@ def set_entry_important(
 
         Args:
             entry (tuple(str, str) or Entry): (feed URL, entry id) tuple.
-            important (bool): Mark the entry as important if true (default),
-                and as unimportant otherwise.
+            important (bool or None): Mark the entry as important if true,
+                as unimportant if false, or as not set if none.
             modified (datetime or None):
                 Set :attr:`~Entry.important_modified` to this.
                 Naive datetimes are normalized by passing them to
@@ -1412,6 +1415,9 @@ def set_entry_important(
             The ``entry`` and ``important`` arguments are now positional-only.
 
         """
+        if important not in (True, False, None):
+            raise ValueError("important should be one of (True, False, None)")
+
         modified_naive: datetime | None
         if isinstance(modified, MissingType):
             modified_naive = self._now()
@@ -1421,9 +1427,7 @@ def set_entry_important(
             modified_naive = modified.astimezone(timezone.utc).replace(tzinfo=None)
 
         feed_url, entry_id = _entry_argument(entry)
-        self._storage.mark_as_important(
-            feed_url, entry_id, bool(important), modified_naive
-        )
+        self._storage.mark_as_important(feed_url, entry_id, important, modified_naive)
 
     def mark_entry_as_important(self, entry: EntryInput, /) -> None:
         """Mark an entry as important.
@@ -1471,17 +1475,12 @@ def _mark_entry_as_dont_care(self, entry: EntryInput, /) -> None:
         """Mark an entry as read and unimportant at the same time,
         resulting in the same read_modified and important_modified.
 
-        This method becoming public is pending on #254.
+        ~This method becoming public is pending on #254.~
 
-        Presumably, we could just use mark_entry_as_{read,important} instead
-        and get the slightly different timestamps,
-        but it's likely better to collect more accurate data.
+        FIXME: As of Jan 2023, this should be replaced by mark_entry_as_unimportant().
 
         """
-        modified_naive = self._now()
-        feed_url, entry_id = _entry_argument(entry)
-        self._storage.mark_as_read(feed_url, entry_id, True, modified_naive)
-        self._storage.mark_as_important(feed_url, entry_id, False, modified_naive)
+        self.mark_entry_as_unimportant(entry)
 
     def add_entry(self, entry: Any, /) -> None:
         """Add a new entry to an existing feed.

src/reader/plugins/mark_as_read.py

@@ -78,7 +78,8 @@ def _mark_as_read(reader, entry, status):
     try:
         for pattern in patterns or ():
             if re.search(pattern, entry.title or ''):
-                reader._mark_entry_as_dont_care(entry)
+                reader.set_entry_read(entry, True, None)
+                reader.set_entry_important(entry, False, None)
                 return
     except EntryNotFoundError as e:
         if entry.resource_id != e.resource_id:  # pragma: no cover

src/reader/types.py

@@ -265,7 +265,8 @@ def feed_url(self) -> str:
     read_modified: datetime | None = None
 
     #: Whether the entry is important or not.
-    important: bool = False
+    #: :const:`None` means not set.
+    important: bool | None = None
 
     #: The date when :attr:`important` was last set by the user;
     #: :const:`None` if that never happened,

tests/test_plugins_entry_dedupe.py

@@ -165,7 +165,7 @@ def test_plugin(make_reader):
     reader.update_feeds()
 
     assert {(e.id, e.read, e.important) for e in reader.get_entries()} == {
-        t + (False,)
+        t + (None,)
         for t in {
             # remain untouched
             (old.id, False),
@@ -264,17 +264,17 @@ def test_plugin_once(make_reader, db_path, monkeypatch, tags):
 
     # nothing changes without tag
     assert {(e.id, e.read, e.important) for e in reader.get_entries()} == {
-        (different_title.id, False, False),
-        (no_content_match.id, False, False),
-        (read_one.id, True, False),
-        (read_two.id, False, False),
-        (read_three.id, False, False),
-        (important_one.id, False, False),
+        (different_title.id, False, None),
+        (no_content_match.id, False, None),
+        (read_one.id, True, None),
+        (read_two.id, False, None),
+        (read_three.id, False, None),
+        (important_one.id, False, None),
         (important_two.id, False, True),
-        (important_three.id, False, False),
-        (unread_one.id, False, False),
-        (unread_two.id, False, False),
-        (unread_three.id, False, False),
+        (important_three.id, False, None),
+        (unread_one.id, False, None),
+        (unread_two.id, False, None),
+        (unread_three.id, False, None),
     }
 
     for tag in tags:
@@ -288,18 +288,18 @@ def test_plugin_once(make_reader, db_path, monkeypatch, tags):
     # 'once' (the strictest) has priority
     if '.reader.dedupe.once' in tags:
         expected = {
-            (different_title.id, False, False),
-            (no_content_match.id, False, False),
+            (different_title.id, False, None),
+            (no_content_match.id, False, None),
             # delete read_one, read_two (older last_updated)
-            (read_three.id, True, False),
+            (read_three.id, True, None),
             # delete important_one, important_two have (older last_updated)
             (important_three.id, False, True),
             # delete unread_one, unread_two (older last_updated)
-            (unread_three.id, False, False),
+            (unread_three.id, False, None),
         }
     else:
         expected = {
-            (different_title.id, False, False),
+            (different_title.id, False, None),
             # delete all others (older last_updated)
             (unread_three.id, True, True),
         }
@@ -489,6 +489,60 @@ def test_read_modified_copying(make_reader, db_path, data, expected, same_last_u
             (9, True, None),
         ],
     ),
+    # None vs False combos (subset of the above)
+    # TODO: find a better way?
+    # none set, modified (last has modified)
+    (
+        [
+            (1, None, datetime(2010, 1, 1)),
+            (9, None, datetime(2010, 1, 9)),
+        ],
+        [
+            (9, None, datetime(2010, 1, 1)),
+        ],
+    ),
+    # old unimportant, modified; new unset
+    (
+        [
+            (1, False, datetime(2010, 1, 1)),
+            (9, None, None),
+        ],
+        [
+            (9, False, datetime(2010, 1, 1)),
+        ],
+    ),
+    # new unimportant, old modified
+    (
+        [
+            (1, None, datetime(2010, 1, 1)),
+            (9, False, None),
+        ],
+        [
+            (9, False, None),
+        ],
+    ),
+    # None vs True combos (subset of the above)
+    # TODO: find a better way?
+    # old important, modified; new unset
+    (
+        [
+            (1, True, datetime(2010, 1, 1)),
+            (9, None, None),
+        ],
+        [
+            (9, True, datetime(2010, 1, 1)),
+        ],
+    ),
+    # new important, old modified
+    (
+        [
+            (1, None, datetime(2010, 1, 1)),
+            (9, True, None),
+        ],
+        [
+            (9, True, None),
+        ],
+    ),
 ]
 
 

tests/test_plugins_mark_as_read.py

@@ -45,7 +45,7 @@ def get_entry_data(**kwargs):
 
     assert len(list(reader.get_entries())) == 4
     assert get_entry_data(read=True) == {
-        (match_new.id, True, datetime(2010, 2, 1), False, datetime(2010, 2, 1)),
+        (match_new.id, True, None, False, None),
     }
 
     parser.entry(1, 3, datetime(2010, 1, 2), title='no match once again')
@@ -54,7 +54,7 @@ def get_entry_data(**kwargs):
     reader.update_feeds()
 
     assert get_entry_data(read=True) == {
-        (match_new.id, True, datetime(2010, 2, 1), False, datetime(2010, 2, 1)),
+        (match_new.id, True, None, False, None),
     }
 
 

tests/test_reader.py

@@ -1569,6 +1569,30 @@ def test_mark_as_unimportant(reader, entry_arg):
     ]
 
 
+def test_set_entry_important_none(reader, entry_arg):
+    reader._parser = parser = Parser()
+    feed = parser.feed(1, datetime(2010, 1, 1))
+    entry = parser.entry(1, 1, datetime(2010, 1, 1))
+    reader.add_feed(feed)
+    reader.update_feeds()
+
+    entry = reader.get_entry(entry)
+    assert entry.important is None
+    assert entry.important_modified is None
+
+    reader._now = lambda: naive_datetime(2010, 1, 1)
+    reader.mark_entry_as_important(entry_arg(entry))
+    entry = reader.get_entry(entry)
+    assert entry.important is True
+
+    reader._now = lambda: naive_datetime(2010, 1, 2)
+    reader.set_entry_important(entry, None)
+
+    entry = reader.get_entry(entry)
+    assert entry.important is None
+    assert entry.important_modified == datetime(2010, 1, 2)
+
+
 @pytest.mark.parametrize(
     'exc', [EntryNotFoundError('feed', 'entry'), StorageError('whatever')]
 )
@@ -2820,6 +2844,24 @@ def test_entry_read_important_modified_remains_set_after_update(reader, flag):
     assert getattr(entry, f'{flag}_modified') == datetime(2010, 1, 1)
 
 
+@pytest.mark.parametrize('value', [None, 2, 'true'])
+@rename_argument('reader', 'reader_with_one_feed')
+def test_set_entry_read_value_error(reader, value):
+    reader.update_feeds()
+    entry = next(reader.get_entries())
+    with pytest.raises(ValueError):
+        reader.set_entry_read(entry, value)
+
+
+@pytest.mark.parametrize('value', [2, 'true'])
+@rename_argument('reader', 'reader_with_one_feed')
+def test_set_entry_important_value_error(reader, value):
+    reader.update_feeds()
+    entry = next(reader.get_entries())
+    with pytest.raises(ValueError):
+        reader.set_entry_important(entry, value)
+
+
 @rename_argument('reader', 'reader_with_one_feed')
 def test_mark_as_dont_care(reader):
     reader.update_feeds()
@@ -2832,9 +2874,9 @@ def test_mark_as_dont_care(reader):
     reader._mark_entry_as_dont_care(entry)
     entry = next(reader.get_entries())
 
-    assert entry.read
-    assert entry.read_modified == datetime(2010, 1, 2)
-    assert not entry.important
+    assert not entry.read
+    assert entry.read_modified is None
+    assert entry.important is False
     assert entry.important_modified == datetime(2010, 1, 2)
 
 

tests/test_storage.py

@@ -574,7 +574,7 @@ def test_important_entry_important(storage):
 
     assert {e.id: e.important for e in storage.get_entries(datetime(2010, 1, 1))} == {
         'one': True,
-        'two': False,
+        'two': None,
     }