Question about el-patch-feature

[haqle314]

Currently, el-patch-feature defines a function to load the feature and add it to el-patch-pre-validate-hook.
Isn't it better to keep track of features to load in a list variable (el-patch--features-to-require) and define a single function (el-patch--require-features) to load all of them? This has the added benefits of el-patch-require-* functions not showing up as candidates for elisp function completion.
What is your thought on this?

[raxod502]

The idea of el-patch-pre-validate-hook is that you can put arbitrary code on it. For example, you might have an el-patch that is conditionally enabled by a minor mode. On el-patch-pre-validate-hook you should then enable that minor mode. That means it is not an elegant design to handle the common case specially. The only actual problem I see with the current situation is the function completion property you mentioned. Do you have a suggestion about how these functions could be named so they would be less intrusive?

[haqle314]

Sorry, I probably didn't explain my point clear enough. Currently, el-patch defines a new function for each feature it needs to load. What I'm asking is why don't we just use a single function instead. Below is a quick code example to demonstrate my solution.

(defvar el-patch--features-to-load nil
  "A list of (feature . require-args) to require before validating patches.")

(defmacro el-patch-feature (feature &rest args)
  `(cl-pushnew (cons ,feature (list ,@args))
              el-patch--features-to-load
              :test #'equal))

(defun el-patch--load-features ()
  (dolist (pair el-patch--features-to-load)
    (apply el-patch-require-function
           (car pair)
           (cdr pair))))

(add-hook 'el-patch-pre-validate-hook 'el-patch--load-features)

[raxod502]

If the el-patch--load-features is only added within el-patch-feature then I am okay with this. Feel free to create a pull request.

[haqle314]

I'm a bit confused, what do you by the function el-patch--load-features is only added within el-patch-feature?
Nevertheless, I wrote this haphazardly only to illustrate my question. Unless I misunderstood something, loading the features is always necessary. So it should be done separately, not as a function in the user-customizable variable el-patch-pre-validate-hook. I will think of something and submit a pull request.

[raxod502]

OK, your analysis and solution seem reasonable to me. It makes sense to not put anything on el-patch-pre-validate-hook, and instead where we call el-patch-pre-validate-hook also iterate through el-patch--features-to-load and invoke el-patch-require-function on them.

[raxod502]

This thread is being closed automatically by Tidier because it is labeled with "waiting on response" and has not seen any activity for 90 days. But don't worry—if you have any information that might advance the discussion, leave a comment and I will be happy to reopen the thread :)