bastibe / annotate.el

Annotate.el
Other
388 stars 20 forks source link

- added a couple of functions to allow an user to change annotation database #69

Closed cage2 closed 4 years ago

cage2 commented 4 years ago

Hi Bastian!

With this PR i tried to address (tangentially, to be honest) the problem exposed in #68.

The proposed modifications supposed to reload the database that contains the annotations and then iterate on all the buffers that uses annotate-mode refreshing the annotation in each buffer.

It is a bit crude but my impression is that works.

Honestly i do not think this package should deal with control version system, like asked in #68, l but i also think that using this function @gopar or other users could be able write some code to get a different annotation file per git branch.

Bye! C.

bastibe commented 4 years ago

Looks good to me!

In general, I am a bit reluctant to add code for fringe use cases that increase the maintenance surface of the project. But seeing that the change is contained in two functions and does not change existing code, it seems harmless enough.

Do you see additional value in this code beyond #68? If not, it might be wise to put it into the FAQ instead of annotate's source code. But I'll leave that decision up to you.

cage2 commented 4 years ago

On Mon, Jun 08, 2020 at 08:21:31AM -0700, Bastian Bechtold wrote:

Hi Bastian!! :)

Looks good to me!

I have updated the documentation and the other file as usual.

Also i have added a little patch to remove the annoying message that was printed about an unsaved modified buffer when the only modification was actually just reloading annotate-mode.

if OK i am going to merge this patch in the next hours.

In general, I am a bit reluctant to add code for fringe use cases that increase the maintenance surface of the project. But seeing that the change is contained in two functions and does not change existing code, it seems harmless enough.

I am surprised too that this modification took so few changes in the code to be effective, this is all Emacs merit of course! :)

For bugs blame me instead! :D

Do you see additional value in this code beyond #68? If not, it might be wise to put it into the FAQ instead of annotate's source code. But I'll leave that decision up to you.

I would never added this feature if not asked but, someway, i think could be useful now. So i am happy that we got the issue report #68; probably this could even become some sort of scaffolding to build some complicated kind of database management (database of annotation, i mean). In my opinion these features (interfacing with git bzr, etc) should not be the goal of this library but i think it is OK to make life easier for people that want to write some code on top of annotate.el.

So i think leaving these functions in the main code could be a general improvement for this package.

Bye!!! C.

bastibe commented 4 years ago

Thank you for your considerations. This looks great! Apart from the version change, feel free to merge when you're ready.

cage2 commented 4 years ago

On Tue, Jun 09, 2020 at 11:49:42PM -0700, Bastian Bechtold wrote:

Hi Bastian!

@bastibe commented on this pull request.

@@ -7,7 +7,7 @@ ;; Maintainer: Bastian Bechtold ;; URL: https://github.com/bastibe/annotate.el ;; Created: 2015-06-10 -;; Version: 0.7.0 +;; Version: 0.7.1

One minor nitpick: In the semantic versioning convention, we should bump the minor version, not the bugfix version.

Right!

In short, the convention says that major version changes denote breaking changes, minor version changes are feature additions, and bugfix version changes only fix bugs.

Since this is a feature addition, it should result in 0.8.0.

That is absolutely true. My fault, :( I am pushing the fix right know.

Thank you again for reviewing the code so carefully, this always results in very precious suggestions!

(Although we might want to postpone that a bit if there are more things you'd like to add before pushing a new version)

I think i am OK, let's see what code can "blossom" from this patch! :)

Bye! C.