Delimiter missing between tags

[jarun]

I have a Firefox bookmark with the tags: hello, language

After auto-import both tags are combined into 1 as language hello

13. Language Tools
   > https://addons.mozilla.org/fr/firefox/language-tools/
   # language hello

Should be: hello,language

The delimiter (comma) is missing between the tags and they are combined into one (in reverse order). You need to use parse_tags().

Also, please ensure that original spaces within tags are maintained (parse_tags()) takes care of this as well. For example: hello world, language tools should show as:

hello world,langauge tools

[alex-bender]

@jarun what the purpose of the is_nongeneric_url function call here?

[jarun]

The function filters out bookmarks those start with place:, file://, apt:.

[alex-bender]

It's little bit strange that parse_tags function accepts one element list with a string formatted as 'first_tag,second_tag' just to split this string and didn't take into account that keywords argument could be list of tags already, so only DELIM.join(keywords) is actually needed.

So here are two possible variants:

1. Fix function which loads firefox tags (already separated) or
2. Fix the parse_tags to take into account that keywords could be already filtered tags. What do we prefer?

[alex-bender]

Also, maybe you've already checked, what if we move this function invocation from here to here?

[jarun]

parse tag does it because when we accept the input from users they can be in the form:

tag 1, tag2 which gives [tag, 1,, tag2] and should yield 2 tags: tag 1,tag2 and NOT 3 tags: tag,1,tag2.

You can't move it anywhere before you actually get the url.

[alex-bender]

Sorry, look 3 lines below: url = res.fetchone()[0]

[alex-bender]

Sorry, seems that I din't get something, because I think that even in this case we can do this:

>> data = ' tag 1,  tag2 , some other '
>> [tag.strip() for tag in data.split(',')]
<< ['tag 1', 'tag2', 'some other']

[jarun]

Your input data is in a list. When user enters:

buku tag 1, tag2, some other you get:

[tag, 1,, tag2,, some, other] as args.

[jarun]

We save the bookmarks in the format:

,tag 1,tag2,some other,

separated and wrapped by DELIM (check delim_wrap() as well).

[alex-bender]

Seems that there are some cases which I don't know. If user enters

In [1]: data = 'buku tag 1, tag2, some other'

In [2]: [tag.strip() for tag in data.split(',')]
Out[2]: ['buku tag 1', 'tag2', 'some other']

With this line we can achieve the same what whole function does: DELIM+DELIM.join([tag.strip() for tag in data.split(',')])+DELIM

[jarun]

~/G/Buku$ gd
diff --git a/buku.py b/buku.py
index 2167236..34f14f4 100755
--- a/buku.py
+++ b/buku.py
@@ -3260,6 +3260,8 @@ POSITIONAL ARGUMENTS:
     # Parse the arguments
     args = argparser.parse_args()

+    print(args.tag)
+
     # Show help and exit if help requested
     if args.help:
         argparser.print_help(sys.stdout)
~/G/Buku$ ./buku.py --tag buku tag 1, tag2, some other
['buku', 'tag', '1,', 'tag2,', 'some', 'other']

[alex-bender]

now I see. What about adding into parsing func one check like if len(keywords) > 1 for cases when tags already separated?

[alex-bender]

@jarun oh, nevermind, I'm gonna fix invoking. It'll be easier. Sorry for distracting

[jarun]

There can be an input like ['world,hello'] too. ;). It should be ,hello,world,. ;)

There are reasons I took my time to stabilize this painstaking function. :dagger:

[alex-bender]

please check this PR https://github.com/jarun/Buku/pull/183

[alex-bender]

There are reasons..

Definitely sure! It looks confusing at the first glance, without deep context knowledge.. Sorry again.

[jarun]

Sorry again.

Don't be! Actually I am glad to find the second person to dive into it. If you can simplify it that would be great.

[alex-bender]

I have some ideas. Let me check. Just want to be sure that i'm covering all usecases. Do we have complete unit test for this function?

[jarun]

I left some questions for you in the PR. Take your time to make any other changes if needed. I will review tomorrow.

Also, once you are back we need to document this. Probably after the support for input file is in. Also, how about renaming -bi to -ai (for auto-import). Has a ring to it for sure. ;)

[alex-bender]

Let's switch to the PR discussion. I don't mind renaming.

[alex-bender]

I'm clothing this due to PR https://github.com/jarun/Buku/pull/183