ADDED new api interfaces for interacting with tags. #17
Conversation
|
Thanks for your contribution! Overall i like this. :-) To begin with, i'll leave specific comments on the code as review notes. |
ebuku.el
Outdated
| (message "Added tag to bookmark"))) | ||
|
|
||
| (defun ebuku--tags-join (tags) | ||
| "Internal function that joins a list of tags into a comma separated string of tags." |
There was a problem hiding this comment.
"Internal function to join a list ..." Also, please wrap docstrings on word breaks so each line is less than 80 characters.
There was a problem hiding this comment.
Changed the sentence a little bit so it's less than 80 characters, my linter was complaining that the first line was not a complete sentence when wrapped.
In my vim setup I usually just have a ruler for the 80 character mark that I can toggle but I'd be curious to hear how you deal with seeing if any lines are longer than that or do you have a linter that can do that?
There was a problem hiding this comment.
i use column-enforce-mode to display any characters beyond the 80-character limit in red. :-)
|
Hey, I just wanted to say THANK YOU for taking the time to give me such great feedback! I really appreciate it, especially with naming and documentation conventions. I greatly value clean and uniform code/documentation so I'm trying to learn the generally accepted code and documentation style for elisp so your comments were really helpful! |
There was a problem hiding this comment.
Sorry i've taken so long to get back to you about this! i took on a new role a couple of months ago, so i've been adjusting to that .... and there's also the pandemic, of course. :-P
To fix the indentation at the places i noted, you could use aggressive-indent-mode (which i personally do when editing ELisp).
| (inhibit-read-only t)) | ||
| (ebuku--tags-add index tag) | ||
| (ebuku-refresh) | ||
| (message "Added tag to bookmark"))) |
| ((listp tags) | ||
| (mapconcat 'identity tags ",")) | ||
| (t | ||
| (error "Tags need to be either a string or list")))) |
There was a problem hiding this comment.
"Input must be a string or list of strings"
| (message "Added tag to bookmark"))) | ||
|
|
||
| (defun ebuku--tags-join (tags) | ||
| "Internal function to join a list of TAGS into a comma separated string." |
There was a problem hiding this comment.
"Internal function to join TAGS into a comma-separated string."
| (error "Tags need to be either a string or list")))) | ||
|
|
||
| (defun ebuku--tags-add (index tags) | ||
| "Internal function to append TAGS to the list of tags on the INDEX." |
There was a problem hiding this comment.
"Internal function to append TAGS to the tags for INDEX."
| "--tag", "+" , (ebuku--tags-join tags))))) | ||
|
|
||
| (defun ebuku--tags-remove (index tags) | ||
| "Internal function to remove TAGS from the list of tags on the INDEX." |
There was a problem hiding this comment.
"Internal function to remove TAGS from the tags for INDEX."
| "--tag", "-" , (ebuku--tags-join tags))))) | ||
|
|
||
| (defun ebuku--tags-set (index tags) | ||
| "Internal function to set TAGS on the INDEX. |
There was a problem hiding this comment.
"Internal function to set tags for INDEX to TAGS."
| (defun ebuku--tags-set (index tags) | ||
| "Internal function to set TAGS on the INDEX. | ||
|
|
||
| This function does not append to the current list of TAGS it replaces it." |
There was a problem hiding this comment.
"This function is destructive: it does not append to the list of tags, but replaces it."
|
|
||
| (defun ebuku--tags-join (tags) | ||
| "Internal function to join a list of TAGS into a comma separated string." | ||
| (cond ((stringp tags) |
There was a problem hiding this comment.
Fix indentation for this cond.
| "Append tag to bookmark at point." | ||
| (interactive) | ||
| (ebuku-update-tags-cache) | ||
| (let ((tag (completing-read "Append tag? " ebuku-tags)) |
There was a problem hiding this comment.
Fix indentation for this let.
|
Heya, Yeah it's a pretty crazy time so no worries about not getting back right away. Thanks for the input, I'll get to this in a month. I just have one work week before going on vacation for three weeks and I can definitely feel myself beeing really worn down. So I'm just going to listen to my body right now. |
Hi,
I was migrating all of my bookmarks to buku and in the migration process I ended up wanting to tag a lot of bookmarks so I created a few api functions for this and since this functionality was not in the module I figured I would push it.
I'm new to emacs and lisp so I'm might have missed some coding conventions or general best practice so any feedback regarding it is appreciated.
Some notes regarding my code:
The
ebuku--call-bukufunction seems to have the option to always write to the buffer enabled so I just wrapped my functions with thewith-temp-bufferfunction to not have to do any bigger changes to the overall code to deal with instances where the output of the function doesn't matter.Also I don't check for any errors in the process since buku doesn't seem to return any exit code other than
0even when it outputs[ERROR]. I suppose I could try to regexp the output, but I couldn't seem to wrap my head around how the elisp dialect of regex works so I figured I would just leave it as it is for now and get your feedback on it or return to this later with fresh eyes.