flexibeast / ebuku

Emacs interface to the buku Web bookmark manager.
92 stars 7 forks source link

Refactoring and new default value for ebuku-buku-path #11

Closed Kungsgeten closed 4 years ago

Kungsgeten commented 4 years ago

Mostly refactoring of ebuku--search-helper. It is now (hopefully) easier to grasp. The majority of its functionality has been broken down to these functions:

ebuku--bookmarks handles the calling of buku, and retrieves the results. It returns the result as a list of data. This coulud potentially be used by other packages, so perhaps the function should be called ebuku-bookmarks (since it isn't really a private function).

ebuku--insert-bookmark-string handles the formatting and insertion of a bookmark into a buffer. ebuku--search-helper calls it when inserting all of the results into the EBuku buffer.

ebuku--get-bookmark-count has been removed, since it was no longer used. It might be the case however that I misunderstand how the original "count" local variable was used in ebuku--search-helper.

flexibeast commented 4 years ago

Thank you for your further contribution! :-)

A few comments:

Unfortunately, the refactoring part of the PR, which i agree with in principle, somewhat clashes with work i did yesterday, and finalised and committed today, to address #5. More specifically, your ebuku--bookmarks function has a name very similar to my ebuku-bookmarks variable for caching; and i'm also wondering whether your ebuku--bookmarks function and my new ebuku-gather-bookmarks function should somehow be combined into a single function, or whether instead they should simply be given better names to indicate their respective purposes. Thoughts?

Kungsgeten commented 4 years ago

You're welcome! I didn't see that you'd made changes until I submitted the PR. Unfortunately I only use git for pretty basic stuff (mostly personal projects). Thus I'm not aware of best practices, sorry. I understand though that separate PRs would be better.

My main intent with the PR was actually to provide better completion support, as the next step.

I'll have a look into your recent changes and will try to submit separate PRs :)