' and ` (non-)pairing setting in lisp modes ignored

ishantar commented 5 years ago

Expected behavior

Pairing behavior of single quote and backtick characters should be adjusted for `sp-lisp-modes' buffers. (specifically, lines 75-91 of smartparens-config.el should actually do something)

Actual behavior

Behavior within `sp-lisp-modes' is not altered. The `sp-lisp-modes' variable is set correctly by Customize, but the expected keymap changes do not occur.

Attempting to execute the smartparens-config.el code directly will succeed if Emacs has previously been forced to load smartparens.el. If Emacs has not had to do so (ie, only autoloads are available), calling `sp-local-pair' will fail with a void-function complaint. Activating the minor mode is sufficient to trigger loading.

The following workaround succeeds if placed in the init file;

(progn  (smartparens-global-mode)
        (sp-local-pair sp-lisp-modes  "'" 'nil :actions 'nil)
        (sp-local-pair sp-lisp-modes  "`" 'nil :actions 'nil)  )

Environment & version information

Full disclosure: while I also tested using Emacs 26.1 (current release, identical results), I initially encountered this bug while using a development branch build (27.0.50). The init file used with 26.1 did include traditional require-package' andpackage-initialize' calls, but was an edited v27 file, and v27 makes changes to the init process. If can share my full init.el if there's trouble reproducing.

smartparens version: 20180705.801
Active major-mode: various including clojure-mode, cider-repl-mode, lisp-interaction-mode, emacs-lisp-mode, c-mode
Emacs version (M-x emacs-version): various including 26.1, 27.0.50
Spacemacs/Evil/Other starterkit (specify which)/Vanilla: Vanilla
OS: Arch Linux

Fuco1 commented 5 years ago

If you require smartparens-config it will also require smartparens at which point all the functions will be defined.

If you only have autoloads, do not load smartparens-config and then try to call sp-local-pair it will probably fail because that function has no autoload definition.

Which one of the above is your case?

Would the solution here be to simply make it autoloadable? I think that should be enough.

ishantar commented 5 years ago

If you only have autoloads, do not load smartparens-config and then try to call sp-local-pair it will probably fail because that function has no autoload definition.

Yes. Those details are mostly me trying to determine whether code from smartparens-config was being evaluated with smartparens.el unloaded. What I didn't know how to diagnose was whether that code was failing or going unexecuted.

If you require smartparens-config it will also require smartparens at which point all the functions will be defined.

I'm not really clear on where smartparens-config is supposed to be required from... smartparens-config does not appear in my selected-packages, and grepping the rest of the package for "smartparens-config" only shows it referenced from autoloads and the rust file. Just so I'm sure, I'm not supposed to be activating the language specific features by adding explicit require calls to my init file... right?

Does the initialization sequence somehow depend on (require 'package) being called?

Would the solution here be to simply make it autoloadable? I think that should be enough.

That might be all it needs.

Fuco1 commented 5 years ago

smartparens-config is not a package but a separate file. If you install smartparens through package.el or something compatible it will be on your load-path so you can require it.

You can also require the language-specific files individually but not all of them call (require 'smartparens) which is probably a bug.

The stupid issue is that the lisp config is in the smartparens-config file which is also the one which sets-up the loading of all other files. We should split it into a separate file to make it independent.

ishantar commented 5 years ago

Adding (require 'smartparens-config) to my init file does seem to fix the issue. Is installing the package via package.el supposed to add such a call?

If you're supposed to activate the config stuff manually, that could be a lot more clear... making it possible from the customize interface would be ideal (especially since apparently there's stuff in there that silently does nothing instead of what it says it does unless you've required an extra file). I don't mean to complain, but I at least found it confusing.

Fuco1 commented 5 years ago

It does exactly what it does and it is explained in the readme. No, package.el is not supposed to add that line.

I'm not sure where the confusion comes from. It's not possible to require packages from customize interface.

ishantar commented 5 years ago

It does exactly what it does and it is explained in the readme

...that's true, my apologies. I suppose my real complaint is with package.el for not installing readme files.

I'm not sure where the confusion comes from.

Basically it was three things:

none of the customization options, installed documentation, or source file commentary refers to requireing smartparens-config at all
while README.md does clearly instruct you to call require 'smartparens-config as part of initializing the package (my bad), in the link to "complete documentation", it isn't made so explicit that this is a mandatory post-installation step if you want access to the default config
consulting the wiki docs led me to:

Getting Started: [...]The process of installing smartparens is as simple as calling package-install. However, if you want to learn in detail what packages smartparens depend on and what settings it modifies in order to function, read the {{installation}} manual...

wherein...

[...] You can also install this as a package with M-x package-install smartparens [...] If you've installed smartparens as a package, you don't need to require it, as there is an autoload on smartparens-global-mode.

... which it seems I misunderstood.

... It's not possible to require packages from customize interface.

As a completely separate issue, there's no way that's actually true... couldn't you have a custom variable hold a list symbols and pass it to require as the subfeature parameter?

Fuco1 commented 5 years ago

About the last point, to run my function to do the requiring I would have to load my package first to have it available. There are ways to do it with custom setters but nobody does that as it's quite magical.

Where are those quotes coming from? Clearly that text needs a bit of an update.

ishantar commented 5 years ago

https://github.com/Fuco1/smartparens/wiki https://github.com/Fuco1/smartparens/wiki/Installation

I don't know much about custom setters but wouldn't something like be fine?

(:set  (lambda  (NAME VALUE)
         (custom-set-default NAME VALUE)
         (funcall 'require 'smartparens NAME)))

(...as the setter for a hypothetical sp-use-subfeatures-list custom variable)

edit: formatting, elisp correctness

Fuco1 commented 5 years ago

org-mode does something like that and it was a terrible idea. It's always broken for everyone because custom.el and all the autoloading things and everything just doesn't work well. The Emacs startup process is very very very very compicated so it's better to avoid anything too goofy.

I will update the docs to make it clearer hopefully for the future.

ishantar commented 5 years ago

Fair enough... thanks for your attention on this issue, and sorry for raising what it now seems to be a pbkac :p

Fuco1 commented 5 years ago

Yea no worries, thanks for the feedback, sometimes I can't quite appreciate the point of view of the user since I have it everything in my head and things are "just obvious".

ishantar commented 5 years ago

You might be right about it incurring too much design debt to actually use, but I guess "not possible" sounded like fun to tackle. In case you find it interesting:

[diff against the 20181028.1005 release version of smartparens.el: A defcustom is added (sp-config-options) which can be set to hold 'smartparens-config, a custom function, both, or nil. Its :set function calls set-default, then also calls setq on a new hook variable (sp-config-hook), using the :set value argument after appropriate transformations. This hook is then ran (first) by each of the base minor-mode functions in a progn. The intent is this way, the config settings will be in place once a minor mode begins activating, but loading 'smartparens won't cause the call to require yet. 'smartparens should necessarily be loaded before its mode functions' real definitions can be executed. So theoretically there shouldn't be major ordering issues/cycles.]

--- ../smartparens.el   2018-11-05 13:27:23.573308743 -0500
+++ ../smartparens.el.patched   2018-11-05 13:25:55.088841916 -0500
@@ -704,6 +704,30 @@
   :group 'editing
   :prefix "sp-")

+(defvar sp-config-hook nil)
+
+(defcustom sp-config-options '(smartparens-config)
+  "Setting this variable directly outside of Customize will not take effect:
+    to configure this option from lisp code, set `sp-config-hook' to either
+    nil (no additional settings), or to a list of functions that each take
+    no arguments.  To use the default configuration, the first of these
+    functions should call `(require 'smartparens-config)'"
+
+  :type '(choice (const "no additional settings" nil)
+                 (set :tag "set configuration options:"
+                      (const :tag "use the default config for language-specific settings" 
+                             smartparens-config)
+                      (function :tag "set a personal configuration function")))
+  :group 'smartparens
+  :set (lambda (NAME VAL)  (let ((c (car VAL)))
+          (prog1 
+            (set-default NAME VAL)
+             (and c (if (functionp c)
+                           (setq sp-config-hook '(c))
+                      (setq sp-config-hook 
+                        (cons (lambda () (require 'smartparens-config))
+                               (and (cadr VAL) (list (cadr VAL))))) ))))))
+
 ;;;###autoload
 (define-minor-mode smartparens-mode
   "Toggle smartparens mode.
@@ -717,13 +741,14 @@
   :lighter (" SP" (:eval (if smartparens-strict-mode "/s" "")))
   :group 'smartparens
   :keymap smartparens-mode-map
+  (progn (run-hooks 'sp-config-hook)
   (if smartparens-mode
       (progn
         (sp--init)
         (add-hook 'self-insert-uses-region-functions 'sp-wrap--can-wrap-p nil 'local)
         (run-hooks 'smartparens-enabled-hook))
     (remove-hook 'self-insert-uses-region-functions 'sp-wrap--can-wrap-p 'local)
-    (run-hooks 'smartparens-disabled-hook)))
+            (run-hooks 'smartparens-disabled-hook))))

 (defvar smartparens-strict-mode-map
   (let ((map (make-sparse-keymap)))
@@ -757,6 +782,7 @@
 after the smartparens indicator in the mode list."
   :init-value nil
   :group 'smartparens
+  (progn (run-hooks 'sp-config-hook) 
   (if smartparens-strict-mode
       (progn
         (unless smartparens-mode
@@ -773,9 +799,11 @@
     (put 'sp-backward-delete-char 'delete-selection 'supersede)
     (put 'sp-delete-char 'delete-selection 'supersede)
     (remove-hook 'self-insert-uses-region-functions 'sp--self-insert-uses-region-strict-p 'local)
-    (let ((std-val (car (plist-get (symbol-plist 'sp-autoskip-closing-pair) 'standard-value)))
-          (saved-val (car (plist-get (symbol-plist 'sp-autoskip-closing-pair) 'saved-value))))
-      (setq sp-autoskip-closing-pair (eval (or saved-val std-val))))))
+             (let ((std-val 
+                      (car (plist-get (symbol-plist 'sp-autoskip-closing-pair) 'standard-value)))
+                   (saved-val 
+                      (car (plist-get (symbol-plist 'sp-autoskip-closing-pair) 'saved-value))))
+               (setq sp-autoskip-closing-pair (eval (or saved-val std-val)))))))

 ;;;###autoload
 (define-globalized-minor-mode smartparens-global-strict-mode
@@ -9343,13 +9371,14 @@
 support custom pairs."
   :init-value nil
   :group 'show-smartparens
+  (progn (run-hooks 'sp-config-hook)
   (if show-smartparens-mode
       (unless sp-show-pair-idle-timer
         (setq sp-show-pair-idle-timer
               (run-with-idle-timer sp-show-pair-delay t
                                    'sp-show--pair-function)))
     (when sp-show-pair-overlays
-      (sp-show--pair-delete-overlays))))
+              (sp-show--pair-delete-overlays)))))

 ;;;###autoload
 (define-globalized-minor-mode show-smartparens-global-mode

Fuco1 / smartparens