bastibe / annotate.el

Annotate.el
Other
384 stars 20 forks source link

Added a command to delete an annotation #113

Closed cage2 closed 2 years ago

cage2 commented 3 years ago

Hi @bastibe !

I have added a new command C-c C-d to delete the annotation under the point in a buffer. This is a small patch as the code to build the command was more or less already there! :)

Sorry, i made a mistake and i am using the master branch instead of a custom branch for this pull request. :(

Bye! C.

cage2 commented 2 years ago

Hi!

I forgot to actually use the variable annotate-annotation-confirm-deletion 😓! I think everything is ready for merging! :)

Bye! C.

bastibe commented 2 years ago

Hi @cage2,

looks good to me! I have added two similar comments, regarding the prompt. Feel free to leave all customization in there, or delete them completely. I can see useful arguments for both paths.

It might be that I am in a deletion-mood, since I have spent the last few weeks excising dead code at work. So my perception might be skewed.

Otherwise, merge when you're ready!

cage2 commented 2 years ago

Hi @bastibe !

Sorry for the late reply!

Yes, very likely the use of annotate-y-or-n-prompt-function could be considered a case study for over engineering! :-D Instead i would prefer to keep the boolean variable to asking for confirmation before deleting an annotation as some users could be more comfortable to keep a "safety net" for annotations (even if they are ephemeral by nature).

Bye! C.

bastibe commented 2 years ago

I must say that if there is one big "lesson" I have learned in my programming, it is to avoid unnecessary complication. Too many times have I seen student projects crumble under the weight of their abstractions. Too often have I seen projects reduce to something much simpler once I removed all the "architecture". I now try to code stuff in the most straight-forward manner first, and only introduce abstractions and indirections when they are absolutely provably necessary. Or at least that's what I try to do.

Anyway, I don't mean this as criticism to this pull request. Emacs is very forgiving with regards to configuration variables, and the prompt is a reasonable and unproblematic customization option. Merge when you're ready!

cage2 commented 2 years ago

Hi @bastibe !

In retrospective i think i have added the customizable variable y-or-n-p just because of my ignorance about elisp, i did not know about defalias for example. Now i wonder if there could be something superfluous hidden in the code that i could remove using simpler or already existing constructs in the standard library.

Anyway, let's merge this, for now! :)

Bye! C.