~pkal/public-inbox

4 2

Re: :bind-into should not eval-after-load

Details
Message ID
<87edbdma8j.fsf@ushin.org>
DKIM signature
pass
Download raw message
Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@ushin.org> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@ushin.org> writes:
>>>
>>>> (setup foo
>>>>   (:bind-into bar-map
>>>>     "a" foo-baz))
>>>>
>>>> macroexpands to
>>>>
>>>> (eval-after-load 'foo
>>>>   #'(lambda nil
>>>>       (define-key bar-map "a" #'foo-baz)))
>>>>
>>>> but I would expect it to macroexpand to
>>>>
>>>> #'(lambda nil
>>>>     (define-key bar-map "a" #'foo-baz))
>>>
>>> I am confused, why should it evaluate to a lambda expression?  Do you
>>> mean
>>>
>>> (eval-after-load 'bar
>>>   ...)
>
> I took a look at :bind-into again, and the main issue here is actually
> that it tries to guess if something is a map or not, by looking at the
> printed representation of a symbol.  Generally speaking, I think this is
> a mistake, and we see the consequences of this approach here.
>
> What is the concrete bar-map here?  Perhaps setting the setup-map symbol
> property could be an alternative solution, that wouldn't involve having
> to deal with this question you raised:
>
>> Thank you. Yes, this makes more sense than the current expansion:
>>
>> (eval-after-load 'foo
>>   ...)
>>
>> However, this raises a nuance.  Should the macro
>>
>> (:bind-into foo-mode-map
>>   ...)
>>
>> expand to
>>
>> (eval-after-load 'foo
>>   ...)
>>
>> or
>>
>> (eval-after-load 'foo-mode
>>   ...)
>>
>> ?
>>
>>>> The documentation for :bind-into lacks an asterisk, indicating that it
>>>> should not be wrapped in with-eval-after-load:
>>>>
>>>>  - (:bind-into FEATURE-OR-MAP &rest REST)
>>>>
>>>> However, because it uses :bind internally, it ends up doing so anyway.
>>>
>>> Hmm, you are right, but renaming it might break backwards compatibility
>>> for one of the dozen of users ;)
>>
>> Hmm... :)
>>
>> Here's a real-world bug from my config:
>>
>> (setup (:if-package bicycle)
>>   (:bind-into outline-minor-mode-map
>>     [C-tab] bicycle-cycle
>>     [backtab] bicycle-cycle-global)
>>   (add-hook 'prog-mode-hook #'outline-minor-mode)
>>   (add-hook 'prog-mode-hook #'hs-minor-mode))
>
> Hmm, here the issue is that the feature is called outline, but the minor
> mode is outline-minor-mode... I am leaning more and more to just mark
> the macro as deprecated and advise against using it.

Yes, unless there's a reliable way to detect in what file a non-autoloaded keymap is
defined before said file is loaded, then :bind-into would need to accept
the file (feature) name as an additional argument.

>> The autoloaded bicycle commands are never actually bound in
>> outline-minor-mode-map because the define-key forms are wrapped in
>>
>> (eval-after-load 'bicycle ...)
>>
>> The only entry points to load bicycle.el are the autoloaded commands (I
>> never explicitly require bicycle.el).  It's a catch-22.  The library is
>> only loaded after the autoloaded commands are invoked, but their
>> keybindings only get bound after the library is loaded.
>>
>> What do you suggest?
>>
>> Joseph

Re: :bind-into should not eval-after-load

Details
Message ID
<87le5kgfw6.fsf@posteo.net>
In-Reply-To
<87edbdma8j.fsf@ushin.org> (view parent)
DKIM signature
pass
Download raw message
Joseph Turner <joseph@ushin.org> writes:

[...]

>>>>> The documentation for :bind-into lacks an asterisk, indicating that it
>>>>> should not be wrapped in with-eval-after-load:
>>>>>
>>>>>  - (:bind-into FEATURE-OR-MAP &rest REST)
>>>>>
>>>>> However, because it uses :bind internally, it ends up doing so anyway.
>>>>
>>>> Hmm, you are right, but renaming it might break backwards compatibility
>>>> for one of the dozen of users ;)
>>>
>>> Hmm... :)
>>>
>>> Here's a real-world bug from my config:
>>>
>>> (setup (:if-package bicycle)
>>>   (:bind-into outline-minor-mode-map
>>>     [C-tab] bicycle-cycle
>>>     [backtab] bicycle-cycle-global)
>>>   (add-hook 'prog-mode-hook #'outline-minor-mode)
>>>   (add-hook 'prog-mode-hook #'hs-minor-mode))
>>
>> Hmm, here the issue is that the feature is called outline, but the minor
>> mode is outline-minor-mode... I am leaning more and more to just mark
>> the macro as deprecated and advise against using it.
>
> Yes, unless there's a reliable way to detect in what file a non-autoloaded keymap is
> defined before said file is loaded, then :bind-into would need to accept
> the file (feature) name as an additional argument.

My proposal would be to start deprecating passing a map to :bind-into:

Re: :bind-into should not eval-after-load

Details
Message ID
<87o7afaxrt.fsf@ushin.org>
In-Reply-To
<87le5kgfw6.fsf@posteo.net> (view parent)
DKIM signature
pass
Download raw message
Philip Kaludercic <philipk@posteo.net> writes:

[...]

> My proposal would be to start deprecating passing a map to :bind-into:
>
> diff --git a/setup.el b/setup.el
> index be1e9bf..cc52b10 100644
> --- a/setup.el
> +++ b/setup.el
> @@ -598,11 +598,13 @@ The first FEATURE can be used to deduce the feature context."
>    :repeatable t)
>
>  (setup-define :bind-into
> -  (lambda (feature-or-map &rest rest)
> -    (if (string-match-p "-map\\'" (symbol-name feature-or-map))
> -        `(:with-map ,feature-or-map (:bind ,@rest))
> -      `(:with-feature ,feature-or-map (:bind ,@rest))))
> -  :documentation "Bind into keys into the map of FEATURE-OR-MAP.
> +  (lambda (feature &rest rest)
> +    (if (string-match-p "-map\\'" (symbol-name feature))
> +        (progn
> +          (warn "The `:bind-into' is considered unreliable, avoid using it if possible.")
> +          `(:with-map ,feature (:bind ,@rest)))
> +      `(:with-feature ,feature (:bind ,@rest))))
> +  :documentation "Bind into keys into the map of FEATURE.
>  The arguments REST are handled as by `:bind'."
>    :debug '(sexp &rest form sexp)
>    :ensure '(nil &rest kbd func)
>
>
> WDYT?

If we are willing to deprecate the ability to pass a map as the first
argument to :bind-into, then I propose that :bind-into somehow accept an
optional MAP argument to override the map derived from the feature name.

With the attached patch, IMO the expansions now DWIM:

(setup bicycle
  (:bind-into outline  ; No MAP specified
      [C-tab] bicycle-cycle))

expands to

(eval-after-load 'outline  ; Correct lazy loading
  (function
   (lambda nil
     (define-key outline-mode-map  ; Good guess at MAP
                 [C-tab] (function bicycle-cycle)))))

while

(setup bicycle
  (:bind-into outline outline-minor-mode-map  ; MAP specified
    [C-tab] bicycle-cycle))

expands to

(eval-after-load 'outline  ; Correct lazy loading
  (function
   (lambda nil
     (define-key outline-minor-mode-map  ; Keys bound is specified MAP
                 [C-tab] (function bicycle-cycle)))))

Questions/concerns about the patch:

- It's not backward-compatible.
- Deprecation should be more gradual, i.e., start with a warning.
- Is there a good way to deduplicate the :with-feature forms?
- Are :indent and :debug correct?
- The docstring is verbose.

Thanks!

Joseph

Re: :bind-into should not eval-after-load

Details
Message ID
<87zftx3dyz.fsf@posteo.net>
In-Reply-To
<87o7afaxrt.fsf@ushin.org> (view parent)
DKIM signature
pass
Download raw message
Joseph Turner <joseph@ushin.org> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
> [...]
>
>> My proposal would be to start deprecating passing a map to :bind-into:
>>
>> diff --git a/setup.el b/setup.el
>> index be1e9bf..cc52b10 100644
>> --- a/setup.el
>> +++ b/setup.el
>> @@ -598,11 +598,13 @@ The first FEATURE can be used to deduce the feature context."
>>    :repeatable t)
>>
>>  (setup-define :bind-into
>> -  (lambda (feature-or-map &rest rest)
>> -    (if (string-match-p "-map\\'" (symbol-name feature-or-map))
>> -        `(:with-map ,feature-or-map (:bind ,@rest))
>> -      `(:with-feature ,feature-or-map (:bind ,@rest))))
>> -  :documentation "Bind into keys into the map of FEATURE-OR-MAP.
>> +  (lambda (feature &rest rest)
>> +    (if (string-match-p "-map\\'" (symbol-name feature))
>> +        (progn
>> +          (warn "The `:bind-into' is considered unreliable, avoid using it if possible.")
>> +          `(:with-map ,feature (:bind ,@rest)))
>> +      `(:with-feature ,feature (:bind ,@rest))))
>> +  :documentation "Bind into keys into the map of FEATURE.
>>  The arguments REST are handled as by `:bind'."
>>    :debug '(sexp &rest form sexp)
>>    :ensure '(nil &rest kbd func)
>>
>>
>> WDYT?
>
> If we are willing to deprecate the ability to pass a map as the first
> argument to :bind-into, then I propose that :bind-into somehow accept an
> optional MAP argument to override the map derived from the feature name.

I'd rather just deprecate the feature for now, and maybe later on we can
consider this.  This would be the first time I am deprecating something
like this for setup.el, so I'd like to take it slow.

That being said, feel free to add your macro to the wiki.

> With the attached patch, IMO the expansions now DWIM:
>
> (setup bicycle
>   (:bind-into outline  ; No MAP specified
>       [C-tab] bicycle-cycle))
>
> expands to
>
> (eval-after-load 'outline  ; Correct lazy loading
>   (function
>    (lambda nil
>      (define-key outline-mode-map  ; Good guess at MAP
>                  [C-tab] (function bicycle-cycle)))))
>
> while
>
> (setup bicycle
>   (:bind-into outline outline-minor-mode-map  ; MAP specified
>     [C-tab] bicycle-cycle))
>
> expands to
>
> (eval-after-load 'outline  ; Correct lazy loading
>   (function
>    (lambda nil
>      (define-key outline-minor-mode-map  ; Keys bound is specified MAP
>                  [C-tab] (function bicycle-cycle)))))
>
> Questions/concerns about the patch:
>
> - It's not backward-compatible.
> - Deprecation should be more gradual, i.e., start with a warning.
> - Is there a good way to deduplicate the :with-feature forms?
> - Are :indent and :debug correct?
> - The docstring is verbose.
>
> Thanks!
>
> Joseph
>
>

-- 
	Philip Kaludercic on peregrine

Re: :bind-into should not eval-after-load

Details
Message ID
<87jzl0pnq8.fsf@ushin.org>
In-Reply-To
<87zftx3dyz.fsf@posteo.net> (view parent)
DKIM signature
pass
Download raw message
Philip Kaludercic <philipk@posteo.net> writes:

[...]

> I'd rather just deprecate the feature for now, and maybe later on we can
> consider this.  This would be the first time I am deprecating something
> like this for setup.el, so I'd like to take it slow.

Sounds good to me!

[...]

Thanks,

Joseph
Reply to thread Export thread (mbox)