Why not make this part of Citar itself?

[minad]

The advantage of the Capf API is that it doesn't incur dependencies, so it could go directly into Citar. Is the provided functionality controversial such that @bdarcus prefers to have it in a separate package? If not, having this inside Citar will be advantageous:

• User friendliness.
• Less work for the MELPA/ELPA maintainers.
• Easier to maintain for the Citar maintainers and you, since everything will be updated in sync.
• Better coordination with other Citar features.

Generally I am all for a modularized package ecosystem where it makes sense. But sometimes packages are too fine grained as it seems to be the case here.

[minad]

I looked at the code - there is really only a single function citar-capf which plays in the hands of my arguments above.

You define a minor mode:

citar-capf/citar-capf.el

Lines 97 to 109 in 59006cb

	;;;; Define Minor Mode
	(define-minor-mode citar-capf-mode
	"Create a global minor mode for `citar-capf'.
	This adds hooks and the citar-capf function to the relevant modes."
	:lighter ""
	:global t
	(cond (citar-capf-mode
	;; add to completion framework (buffer-local)
	(add-hook 'completion-at-point-functions #'citar-capf -90 t)
	(add-to-list 'completion-at-point-functions #'citar-capf))
	(t
	(remove-hook 'completion-at-point-functions #'citar-capf)
	(remove #'citar-capf completion-at-point-functions))))

I think such a mode should be avoided, since activating it will only load the package more eagerly. For the user it is not a big difference to activate a mode or to add an auxillary Capf function to the completion-at-point-functions list. See also my Cape and Tempel packages where I only provide autoloaded Capfs without any additional modes.

[bdarcus]

Is the provided functionality controversial such that @bdarcus prefers to have it in a separate package?

@minad - no; I was pretty agnostic, and @mclearc asked if I was OK with it (I gave him part of the code, or at least an idea how to make it work), and so I said sure.

But I didn't think about one or two of your points, and am also fine to move it back too.

[bdarcus]

BTW, someone on the doom discord mentioned all capfs, including this, that use :exclusive no are impacted by this Emacs bug???

If that's true, do you know of a workaround?

[minad]

BTW, someone on the doom discord mentioned all capfs, including this, that use :exclusive no are impacted by this Emacs bug??? If that's true, do you know of a workaround?

Yes, non-exclusive Capfs are affected by this issue in default completion. But it is not a serious issue. I talked to Stefan Monnier about this and he even considers this behavior somewhat intentional, but advises against using non-exclusive Capfs.

The issue is that non-exclusive Capfs must match the prefix such that they are taken into consideration. This means the completion style is ignored and non-exclusive Capfs may be falsely rejected for more complex completion style configurations (e.g. Orderless). But even then this is not a serious issue since the input prefix usually matches some of the candidates when you trigger completion (via TAB or automatically).

Anyway, Corfu contains a workaround for this issue such that the completion styles are always taken into account, and such that the issue can be considered fixed completely if you use Corfu.

https://github.com/minad/corfu/blob/46825deb5268355cb97396b270d83b24c7eee591/corfu.el#L1230-L1255

Additionally, I recommend to extract the Capf properties to a separate variable such that users can adjust the configuration (e.g., :exclusive, :annotation-function, ...) to their liking. For example:

https://github.com/minad/cape/blob/67c48f1c3609f1426f02c5d8b81e23b1e8b89659/cape.el#L229-L233

[mclearc]

Yeah @minad I have no strong feelings about this -- happy to integrate it into citar. @bdarcus let me know how (or whether) you'd like to do this.

[bdarcus]

@mclearc - could you see about a preparing a PR against this branch, with just the function, and incorporating @minad's other suggestion?

That would have the advantage that when I merge breaking changes, this code will work. Plus you can give us feedback ;-)

Or you could wait until we merge that, likely sometime over the next week.

[mclearc]

Closed via emacs-citar/citar#629

[meliache]

As this has been merged into citar itself, I would recommend archiving this repository and add a README message why, explaining that it has been merged. Today I read the 2022-06-13 emacs news, saw the announcement of this package and installed it at first, not knowing that the code is now in citar itself. While configuring it I looked at the issues and found this.

[mclearc]

Yeah that's the plan!