Return None instead of -1 on failure (when success result is a bookmark ID)

[LeXofLeviafan]

As of now, several BukuDb methods (get_rec_id(), get_max_id(), add_rec()) return a bookmark ID on success, but on failure they return -1. I suggest changing their failure value to None.

Reasoning:

• Python is not C so None is always a valid return value
• None is basically meant for this kind of purpose
• the row numbers start with 1 so returning a falsey value on failure would make the success checks more natural (i.e. if buku.add_rec(…))
• even the explicit value comparison becomes more transparent (is None instead of == -1)
• (edit) -1 is an idiomatically valid index value in Python, which makes it even worse of a fit for "no index" value for this language.

[rachmadaniHaryono]

im undecided on this

the reasoning seem good but it may break other program that use buku

but after checking related buku project, not a lot of program use buku with python

also the next release should be major version, so maybe just go for it?

i havent check bukuserver yet

[LeXofLeviafan]

but after checking related buku project, not a lot of program use buku with python

Might be because of this 😅

[jarun]

Let's do the change. We'll document this in the release notes.

[LeXofLeviafan]

…I found a few other things which can be improved in the source of the main buku file, so I'm opening respective issues to discuss them. I suggest fixing them along with this one.

[LeXofLeviafan]

…Tried my hand at implementing the suggested changes (for this and other issues on buku API). You can check it out committed here.

This includes: returning None as "no ID" value, returning [] as "empty search results" value, removing Optional hints where not necessary, changing the type of immutable parameter to bool (or Optional[bool]), making BookmarkVar a named tuple. (…Also simplified a few conditionals where it makes sense.)

And yes, I've made sure all the tests pass (aside from the network one blocked for me by a remote server), and both CLI and GUI appear to be working (can't say I did a lot of manual checks though).

[rachmadaniHaryono]

as note #651 this is another refactor issue rejected

i thought with new release some refactor will be accepted but only this issue based on #648 (comment)

e: and this also accepted #654

[LeXofLeviafan]

as note #651 this is another refactor issue rejected

So far it's the only one rejected actually. #653 was never commented on so far, and the other two appear to have been accepted.

(If this comment is regarding the contents of my sample refactor commit, I'm waiting for feedback on #653 ATM)

[rachmadaniHaryono]

i miss that 653

i agree on the issue because it dont touch code structure at all

do you want to make pr for this? if yes also edit the docstring so type is only on code type hint

e: not sure where to put this but if you import something from typing use import typing as T instead

[LeXofLeviafan]

Well the word "optional" in the docstring can be interpreted as aimed at the parameter itself rather than the value type, so I figured there's no issue keeping it as it is.

As for typing, I've kept the existing import statement rather than making a single type hint differently from all others (or replacing all of them across the entire 6k LOC).

do you want to make pr for this?

I can make one, but only after #646 is closed – to avoid merge conflicts (it involves checking the same -1/None value)

[rachmadaniHaryono]

Well the word "optional" in the docstring can be interpreted as aimed at the parameter itself rather than the value type, so I figured there's no issue keeping it as it is.

i disagree with this.

1. user should understand that parameter is optional when there is default value for that parameter. there maybe time when this is not the case but if that happen just explaint it on docstring instead of putting optional on docstring
2. beginner python may confused between docstring optional and typing.optional

As for typing, I've kept the existing import statement rather than making a single type hint differently from all others (or replacing all of them across the entire 6k LOC).

dont worry about it, just make it with minimum effort as possible