search

bastibe / annotate.el

Annotate.el
Other
384 stars 20 forks source link

Should annotations files with no annotations be saved? #104

Closed indigoviolet closed 3 years ago

indigoviolet commented 3 years ago

I noticed the source code appears to prevent this, but I see files containing no annotations like this

(("~/dev/retinanet-examples/csrc/cuda/utils.h" nil "5d6550d74a31d9b6987c158b8773dca3"))

Note that I have customized annotate-file to be local to the file. Is this a bug?

cage2 commented 3 years ago

Hi!

Very good catch! Fixed in #102 (or so i hope!). ;-)

Bye! C.

indigoviolet commented 3 years ago

@cage2 - Thank you for your work on this! I just pulled #102 and tested this, and now I see that the annotation file is still saved, but it contains the text nil.

cage2 commented 3 years ago

Hi @indigoviolet !

I have checked the issue but the last version of the package seems not to be affected by this problem, may i ask to recheck using this use case?

  1. set the database annotation file targeting another file, or just make a backup of the db
(setq annotate-file "/a/file")

  1. annotate a file

  2. save the annotated file

  3. check that the annotation database contains an entry like:

(("~/an/annotated/file" ((90530 90548 "annotation text" "annotated-text")) "hex-digits"))
  1. delete the annotation from the annotated file
  2. save again the (no more) annotated file
  3. the annotation database file should contains just nil

Thank you!
C.

indigoviolet commented 3 years ago

@cage2 That is the behavior I see as well.

But I think it is a bit inconvenient even to have nil in the annotation database file, because it means that if I open a file with annotate-mode on, then saving that file will lead to an empty annotation database file even if I never added any annotations.

So there are two cases:

  1. I never added any annotations and saved a buffer with annotate-mode on -> ideally no database file should be created
  2. I removed all annotations from a buffer that previous had them, and the annotation database file was previously created -> in this case I don't feel that strongly but it would be nice to remove the ("empty" == nil containing) annotation database file.

Anyway, I'm also fine with shipping #102 as is for now - you have improved a lot of things in that PR! This is not such a big deal for me.

cage2 commented 3 years ago

Hi!

@cage2 That is the behavior I see as well.

Sorry i did not read your previous message properly! :(

But I think it is a bit inconvenient even to have nil in the annotation database file, because it means that if I open a file with annotate-mode on, then saving that file will lead to an empty annotation database file even if I never added any annotations.

So there are two cases:

  1. I never added any annotations and saved a buffer with annotate-mode on -> ideally no database file should be created

  2. I removed all annotations from a buffer that previous had them, and the annotation database file was previously created -> in this case I don't feel that strongly but it would be nice to remove the ("empty" == nil containing) annotation database file.

Well honestly i would prefer to not overload meanings of the existence of a file as synonym for "empty database", we could rather use this for: "this is the first time annotate mode has been activated". But, anyway, i think my preference is matter of personal tastes as my arguments are pretty weak as you can see :)

Anyway, I'm also fine with shipping #102 as is for now - you have improved a lot of things in that PR! This is not such a big deal for me.

Thank you! I am going to merge the branch in a minute. C.

cage2 commented 3 years ago

fixed by #102

indigoviolet commented 3 years ago

Could we leave this open? I still think it's inconvenient for empty database files to be created when no annotations have been created - perhaps it's helpful to explain why:

In this scenario, every single file I open and save creates an empty .annotate file, which I would like to avoid.

cage2 commented 3 years ago

Hi!

Sure, i am reopening the issue! What you have written seems quite valid points to me. 👍

Bye! C.

cage2 commented 3 years ago

Hi @indigoviolet !

May i ask to check if PR #107 solve this issue?

Thank! C.

indigoviolet commented 3 years ago

will try and check soon, thank you!

On Fri, May 7, 2021, 6:39 AM cage2 @.***> wrote:

Hi @indigoviolet https://github.com/indigoviolet !

May i ask to check if PR #107 https://github.com/bastibe/annotate.el/pull/107 solve this issue?

Thank! C.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bastibe/annotate.el/issues/104#issuecomment-834400179, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA32WTXIEEVY2V64WLBH6LTMPUR3ANCNFSM43G755KA .

cage2 commented 3 years ago

Hi!!

will try and check soon, thank you!

Good! Looking forward for your comments!

Bye! C.

cage2 commented 3 years ago

Hi!

Fixed by #107

Bye! C.