support overriding fetched tags when adding a bookmark #775

LeXofLeviafan · 2024-08-30T03:10:06Z

Addressing the case discussed in #759

When running an --add command, tag lists supplied via --tag or --add (2nd value and further) can be prepended with + or -, same as when editing tags
+ means tags in the list should be included in the resulting record tags
- means tags in the list should be excluded from the resulting record tags (this is applied last)
If either of the tag lists is non-empty (even if it's just a , or a blank string), but doesn't start with +/-, it has same effect as + except it overrides fetched tags
These rules apply to both tag lists identically (if both/neither start with -, they're simply combined)
If there's no reason to fetch remote data (i.e. user supplied title and description, applied tags override, and did not request any network tests), the fetch will be skipped

Also added a few CLI tests (mainly related to the changes)

LeXofLeviafan · 2024-08-30T03:11:59Z

buku

 like_escape = lambda s, c='`': s.replace(c, c+c).replace('_', c+'_').replace('%', c+'%')

+def taglist_str(tag_str, convert=None):
+    tags = taglist(tag_str.split(DELIM))
+    return delim_wrap(DELIM.join(tags if not convert else taglist(convert(tags))))


Optionally applying a conversion to the processed tags before converting them back to a sting

LeXofLeviafan · 2024-08-30T03:13:04Z

buku

-            del_error: Optional[Set[int] | range] = None) -> int:  # Optional[IntSet]
+            del_error: Optional[Set[int] | range] = None,
+            tags_fetch: bool = True,
+            tags_except: Optional[str] = None) -> int:  # Optional[IntSet]


New parameters for add_rec()

The method has no * enforcing keyword arguments, so I added them to the end of the list

LeXofLeviafan · 2024-08-30T03:14:21Z

buku

@@ -735,8 +740,11 @@ class BukuDb:
        title_in : str, optional
            Title to add manually. Default is None.
        tags_in : str, optional
-            Comma-separated tags to add manually.
-            Must start and end with comma. Default is None.
+            Comma-separated tags to add manually, instead of fetching them. Default is None.


Must start and end with comma.

This is not a condition that is actually observed, even in the tests for this method 😅

LeXofLeviafan · 2024-08-30T03:15:35Z

buku

    """Format and get tag string from tokens.

    Parameters
    ----------
    keywords : list
        List of tags to parse. Default is empty list.
+    edit_input : bool
+        Whether the taglist is an edit input (i.e. may start with '+'/'-').
+        Defaults to False.


Added support of the +/- prefix to parse_tags() for convenience

LeXofLeviafan · 2024-08-30T03:17:50Z

buku

        return DELIM

-    tags = DELIM
+    if edit_input and keywords[0] in ('+', '-'):
+        return keywords[0] + parse_tags(keywords[1:])


There's several changes but this is the only one with an effect (specifically when edit_input=True supplied)

This is based on how parse_tags() is used in respective places within main()

LeXofLeviafan · 2024-08-30T03:19:36Z

buku

@@ -5668,7 +5685,7 @@ def parse_range(tokens: Optional[str | Sequence[str] | Set[str]],  # Optional[st


 # main starts here
-def main():
+def main(argv=sys.argv[1:], *, program_name=os.path.basename(sys.argv[0])):


This does not affect current usage, but allows to invoke main() without relying on sys.argv (i.e. for testing)

Note that slicing is necessary for that purpose (since ArgumentParser.parse_args() ignores the 1st value in the list only if the list is sys.argv)

LeXofLeviafan · 2024-08-30T03:23:40Z

buku


    # Show help and exit if help requested
    if args.help:
-        argparser.print_help(sys.stdout)
+        argparser.print_help()


sys.stdout is the default value of the argument

LeXofLeviafan · 2024-08-30T03:25:01Z

buku

@@ -5972,7 +5990,7 @@ POSITIONAL ARGUMENTS:
    browse.override_text_browser = False

    # Fallback to prompt if no arguments
-    if len(sys.argv) <= 2 and (len(sys.argv) == 1 or (len(sys.argv) == 2 and args.nostdin)):
+    if argv in ([], ['--nostdin']):


(…What was the point of the 1st check if only 2 specific argument lists are allowed anyway? 😅)

…Incidentally, I believe this could be moved higher (possibly before the piped_input() call, thus also allowing for a simpler condition in there) 🤔

LeXofLeviafan · 2024-08-30T03:28:45Z

buku

+        keywords_except, keywords = [], args.add[1:]
+        # taglists are taken from --add (starting from 2nd value) and from --tags
+        # if taglist starts with '-', its contents are excluded from resulting tags
+        # if BOTH taglists is are either empty or start with '+'/'-', fetched tags are included


Ended up changing the logic here a little; I had to think hard what would make sense to do here, so I decided to elaborate in a comment and made sure to test it thoroughly as well.

LeXofLeviafan · 2024-08-30T03:29:59Z

buku


+        tags, tags_except = parse_tags(keywords), parse_tags(keywords_except)
+        tags, tags_except = ((s if s and s != DELIM else None) for s in [tags, tags_except])


Replacing blank tag lists with None is optional, but I did it anyway for sake of consistency

LeXofLeviafan · 2024-08-30T03:31:12Z

buku

-            bdb.add_rec(url, title_in, tags, desc_in, _immutable(args), delay_commit=False, fetch=not args.offline,
-                        url_redirect=args.url_redirect, tag_redirect=tag_redirect, tag_error=tag_error, del_error=del_error)
+            network_test = args.url_redirect or tag_redirect or tag_error or del_error
+            fetch = not args.offline and (network_test or tags_add or title_in is None)


fetch is calculated in main()

LeXofLeviafan · 2024-08-30T03:33:21Z

tests/test_buku.py

-        ("tag1, tag2", ",t a g 1,t a g 2,"),
+        ([""], DELIM),
+        ([","], DELIM),
+        (["tag1, tag2"], ",tag1,tag2,"),


The docs specify it must be a list… and what sort of logic is "put spaces after every character in tag names" anyway? 😅

LeXofLeviafan · 2024-08-30T03:35:28Z

tests/test_buku.py

@@ -114,14 +114,17 @@ def test_get_PoolManager(m_myproxy):
        ),
    ],
 )
-def test_parse_tags(keywords, exp_res):
+@pytest.mark.parametrize('prefix', [None, '', '+', '-'])


Testing for edit_input (None means False, others mean True with the value itself defining what prefix is expected before the list)

LeXofLeviafan · 2024-08-30T03:37:00Z

tests/test_bukuDb.py

@@ -793,7 +793,7 @@ def test_close_quit(self):
    # self.fail()


-@pytest.mark.parametrize('status', [None, 200, 302, 307, 404, 500])
+@pytest.mark.parametrize('status', [None, 200, 302, 308, 404, 500])


…I figured that at least one permanent redirect HTTP code is necessary to test how add_rec() handles it 😅

LeXofLeviafan · 2024-08-30T03:42:54Z

tests/test_bukuDb.py

+     'except,bar,tags,there,foo', ',baz,fetched,foo bar,have been,http:redirect,qux,some,'),
+    (308, 'there,have been,some,tags,fetched', 'foo,foo bar,qux,bar,baz',
+     'except,http:redirect,bar,tags,there,foo', ',baz,fetched,foo bar,have been,qux,some,'),
+])


Verifying various combinations of supplied tag arguments, along with fetch result (None means fetch=False)

Tag override doesn't apply to network test tags (these are opt-in anyway), but they can be filtered out via tags_except (e.g. tags_except='http:500' would allow to exclude HTTP 500 error tags from generated output, since these tend to be transient)

LeXofLeviafan · 2024-08-30T03:44:37Z

tests/test_cli.py

+def test_version(BukuDb, piped_input, capsys):
+    with pytest.raises(SystemExit):
+        buku.main(['--version'])
+    assert capsys.readouterr().out.splitlines() == [buku.__version__]


LeXofLeviafan · 2024-08-30T03:45:36Z

tests/test_cli.py

+@pytest.fixture
+def piped_input():
+    with mock.patch('buku.piped_input') as fn:
+        yield fn


Technically I could use mock.patch() as a decorator, but these are harder to name

LeXofLeviafan · 2024-08-30T03:46:56Z

tests/test_cli.py

+
+@pytest.fixture
+def bdb(BukuDb):
+    yield BukuDb.return_value


The BukuDb fixture can be used when testing invocation of constructor/static methods; otherwise bdb is more convenient

LeXofLeviafan · 2024-08-30T03:49:16Z

tests/test_cli.py

+     'tags_in': ',baz,qux,', 'tags_except': ',bar,baz,foo,'},
+    {'add_tags': ['+', 'foo,baz,', 'bar,'], 'tag': ['-', 'baz,', 'qux,'],
+     'tags_in': ',bar,baz,foo,', 'tags_except': ',baz,qux,'},
+])


That's 84 test cases… which is still way better than ~3000 that I got when I tried out all combinations (…and I wasn't even done writing them 😆)

LeXofLeviafan · 2024-08-30T03:52:06Z

tests/test_cli.py

+    if del_error:
+        argv += ['--del-error'] + del_range
+    with pytest.raises(SystemExit):
+        buku.main(argv)


Yanno, maybe we should close the DB without calling sys.exit(), at least from within main() – that would make it a better fit for external use 😅

LeXofLeviafan · 2024-08-30T03:54:23Z

tests/util.py

+        buffer.seek(0, os.SEEK_END)
+        return buffer.write(text)
+    finally:
+        buffer.seek(pos)


This isn't really used as of yet, but it could come in handy when testing the stdin fixture (…which could've been placed here as well, but pylint doesn't like imported fixtures)

LeXofLeviafan · 2024-08-30T03:56:46Z

tests/test_cli.py

+        tags_fetch=tags_fetch, tags_except=tags_except, url_redirect=url_redirect,
+        tag_redirect=tag_redirect, tag_error=tag_error, del_error=del_error)
+    bdb.searchdb.assert_not_called()
+    prompt.assert_not_called()


These are mostly due to initial issues I was having with main() (…which were basically fixed by making sys.argv[1:] the default value; but I left this in just to be sure)

jarun · 2024-08-30T12:37:00Z

Thank you!

support overriding fetched tags when adding a bookmark

d20de36

LeXofLeviafan marked this pull request as ready for review August 30, 2024 03:10

LeXofLeviafan commented Aug 30, 2024

View reviewed changes

jarun merged commit 989c447 into jarun:master Aug 30, 2024
1 check passed

LeXofLeviafan deleted the fetched-tags-override branch August 30, 2024 18:02

LeXofLeviafan mentioned this pull request Sep 6, 2024

implementing search-with-markers #777

Merged

github-actions bot locked and limited conversation to collaborators Jan 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support overriding fetched tags when adding a bookmark #775

support overriding fetched tags when adding a bookmark #775

LeXofLeviafan commented Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024 •

edited

Loading

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

LeXofLeviafan Aug 30, 2024

jarun commented Aug 30, 2024


		tags, tags_except = parse_tags(keywords), parse_tags(keywords_except)
		tags, tags_except = ((s if s and s != DELIM else None) for s in [tags, tags_except])

support overriding fetched tags when adding a bookmark #775

support overriding fetched tags when adding a bookmark #775

Conversation

LeXofLeviafan commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeXofLeviafan Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarun commented Aug 30, 2024

LeXofLeviafan Aug 30, 2024 •

edited

Loading