Fixed a bug that allowed to create an annotation with an empty text.

[cage2]

Hi Bastibe!

I think i introduced a regression in the near past because i do not think i met this bug until now. If you try to add an annotation and pass to the minibuffer an empty string as annotation text an error occurs, fontification fails and a text is annotated without no actual text of the annotation.

To prevent this annotate-create-annotation now signals a condition (error) if is asked to create an annotation with no annotation text. The caller supposed to "catch" this condition and manage the condition in a meaningful way (so far: print an error message to the user ;-) )

I have added a a minimal hierarchy of custom errors annotate-error as parent and annotate-empty-annotation-text-error as child.

Bye!! C.

[bastibe]

Hi Cage!

Good catch with that bug!

Currently, you are raising an exception when an annotation with an empty text is created. When an annotation is changed to an empty annotation, however, we interpret that as the user wanting to delete the annotation. It might be more consistent to do the same thing for creation, and do not create an annotation. Or do you see a use case for catching that exception?

[cage2]

Hi Cage!

Hi bastibe!!

Good catch with that bug!

It was sitting there for month i guess, i was surprised i did not step into it until now! :)

Currently, you are raising an exception when an annotation with an empty text is created. When an annotation is changed to an empty annotation, however, we interpret that as the user wanting to delete the annotation. It might be more consistent to do the same thing for creation, and do not create an annotation. Or do you see a use case for catching that exception?

Let's me see if i have interpreted your question correctly (always my bad English :/)

This fragment of code:

                   (condition-case error-message
                       (annotate-create-annotation start end annotation-text nil)
                     (annotate-empty-annotation-text-error
                      (user-error "Annotation text is empty.")))))))

is equivalent to the following:

(if (annotate-string-empty-p annotation-text)
    (user-error "Annotation text is empty.")
  (annotate-create-annotation start end annotation-text nil))

is the code just above what you suggest?

No annotation is created in both case, in principle i would prefer the first because, in the future the function annotate-create-annotation could signal more different conditions and catching a new condition could be added to existing code with no efforts.

But the code in the second snippet appears more readable so it does makes sense to me too, i can not decide which one i prefer! :D

Bye!! C.

[bastibe]

Sorry for being unclear.

I was wondering if you have any plans for the exceptions? In other user-error cases, we either ignored errors, or printed a warning message. But we didn't raise an exception.

So why raise an exception here instead of ignoring the error? I am not against raising an exception. Just wondering why we would raise one here, and not elsewhere.

[cage2]

On Mon, Jul 27, 2020 at 02:45:47AM -0700, Bastian Bechtold wrote:

Hi Bastian!

Sorry for being unclear.

No problem, no problem at all! My fault instead, i think i supposed to attend some English course someday! :)

I was wondering if you have any plans for the exceptions? In other user-error cases, we either ignored errors, or printed a warning message. But we didn't raise an exception.

So why raise an exception here instead of ignoring the error? I am not against raising an exception. Just wondering why we would raise one here, and not elsewhere.

Yes I think i see now! Well as i tried to use this package as sort of a library (see #68) i thought that swallowing the invalid (i mean empty) text in `annotate-create-annotation' is OK for code that interact with the user but if i call that function from (for example) another package, as a developer i need to know if the annotated was created with success or not and, if not, what is the cause of the error. I could use returning value but i think using return values needs more efforts to be as expressive as exceptions.

You are right that we need be consistent in that. I did not checked the code carefully for functions that could benefit from signalling exception (probably as you wrote, the code to change an annotation is a good first candidate) but i am going to take a look!

Bye! C.

[bastibe]

Good idea about separating our error signaling mechanisms in user-facing functions (print errors or ignore them silently), and developer-facing functions (raise exceptions). I like that!

[cage2]

On Wed, Jul 29, 2020 at 12:19:06AM -0700, Bastian Bechtold wrote:

Hi Bastian!

Good idea about separating our error signaling mechanisms in user-facing functions (print errors or ignore them silently), and developer-facing functions (raise exceptions). I like that!

I am happy that you are OK with this idea! :)

What the next step is going to be? Should this PR be merged or it is better that i give a scan to the function adding signalling exception where seems good to do so? I have no preference.

Bye! C.

[bastibe]

I don't have a preference, either. But since this PR is ready to merge already, how about we merge, and do the signaling change separately?

[cage2]

Hi @bastibe !

Sure i am going to follow your suggestion! Merging this one and starting a new PR to explore which are the functions that could signal errors.

Bye! C.