#49 Don't flyspell links

[TobiasZawada]

• Let font-lock put the adoc-flyspell-ignore property on links
• use this in adoc-flyspell-p to decide to check word at point or not
• adoc-flyspell-p is the flyspell-mode-predicate property of adoc-mode

[bbatsov]

The prefix for the commit should be [Fix #49].

This needs a changelog entry, as it can be considered a bug-fix.

[bbatsov]

Why is this needed? Seems kind of redundant to me.

[TobiasZawada]

The Jit-Lock version of Font-lock is free to fontify only the visible part of the buffer if large yanks of text are inserted. flycheck-buffer on the other hand does not care about visibility. So we ensure that the adoc-flycheck-ignore property is correctly applied by font-lock-ensure.

[bbatsov]

That would come with a performance hit, though, that's why I was wondering if it's really needed. I'm also not sure how often people use flyspell-buffer - e.g. I never use it myself.

I think at the very least we should add some note explaining the need for this, as I don't think that's obvious.

[TobiasZawada]

The performance hit is minimized by that junk of jit-lock-fontify-now:

	   (setq next (or (text-property-any start end 'fontified t)
			  end))

           ;; Avoid unnecessary work if the chunk is empty (bug#23278).
           (when (> next start)

[bbatsov]

This null check also seems redundant.

[bbatsov]

(I assume this will work just fine without explicit conversion into boolean, given that Elisp doesn't have a real boolean type anyways)

[TobiasZawada]

I assume that you know that null is the internal command for negation. (You can interpret it as predicate, nullp would be a more appropriate name.)

The flyspell-mode-predicate should return t if the word is to be checked. We mark the words that should not be checked with adoc-flyspell-ignore.

subr.el has a alias not for it if this is more to your liking.

By the way, I already added 1- to check really the inside of the word for adoc-flyspell-ignore. The new version is (null (get-text-property (1- (point)) 'adoc-flyspell-ignore))

[bbatsov]

Should be it be t specifically or just something truthy (anything that's not nil)? I'm asking mostly because I've rarely seen APIs that rely specifically on t as the return value.

[TobiasZawada]

The null returns t on nil. But, that does not really matter. Any non-nil value would do.

[bbatsov]

I misread the intention of adoc-flyspell-p - thought this was the things to ignore, but it's actually the things to check. That being said my initial remark was also because of some style conventions in Elisp:

null is the same as not; both functions are included for the sake of clarity. As a matter of style, it is customary to use null to check whether something is the empty list and to use not to invert the sense of a logical value.

That's why I called null a predicate, despite its name. Anyways, not a big deal.

[TobiasZawada]

... my initial remark was also because of some style conventions in Elisp:

null is the same as not; both functions are included for the sake of clarity. As a matter of style, it is customary to use null to check whether something is the empty list and to use not to invert the sense of a logical value.

Good point. I've changed it.

[bbatsov]

Btw, did you compare your approach with this one https://github.com/jrblevin/markdown-mode/blob/master/markdown-mode.el#L2330 ? I'm wondering why markdown-mode is doing something different.

[TobiasZawada]

Btw, did you compare your approach with this one https://github.com/jrblevin/markdown-mode/blob/master/markdown-mode.el#L2330 ? I'm wondering why markdown-mode is doing something different.

I checked only after your remark.
They do something similar but not the same.
They assume that the faces themselves determine whether words should be checked or not. IMO it is better to put adoc-flyspell-ignore as an extra text property on the places to be ignored. You have all the info the moment you fontify. In that moment you can really decide what to check and what not. Later on this info is compressed into the faces. It is not clear anymore why exactly the face was put onto the text.

[bbatsov]

Fair enough. Let's go with your approach then.

Just mention it in the changelog and we can merge this.

[TobiasZawada]

@bbatsov In our company we were told just to write the issue title in the "Bugs fixed" section. This is also reasonable, when one considers that this section lists the fixed things.

Do you agree?

Another remark: There will be certainly more things that shouldn't be checked by Flyspell. These are new issues which we handle when we cross them.