Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Stefan Monnier]

>   * lisp/calculator.el: Lots of revisions
  
>   - Kill the calculator buffer after electric mode too.
>   - Make decimal mode have "," groups, so it's more fitting for use in
>     money calculations.
>   - Factorial works with non-integer inputs.
>   - Swallow less errors.
>   - Lots of other improvements, but no changes to custom variables, or
>     other user visible changes (except the above).

The above description is the kind of thing that should go into etc/NEWS.

But you forgot to include a ChangeLog entry for that commit.  For the
ChangeLog entry, you should instead include details about which parts of
the code you changed (see
https://www.gnu.org/prep/standards/html_node/Change-Logs.html).  You can
use "C-x 4 a" (both from the calculator.el and the *vc-diff* buffers) to
help you write that.

And the commit message should be a textual copy of the ChangeLog entry
just with the leading TABs stripped (you can use C-c C-a from the
*vc-log* buffer to let Emacs fetch that ChangeLog text for you).


        Stefan

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Eli Barzilay]

Yesterday, Stefan Monnier wrote:
> >   * lisp/calculator.el: Lots of revisions
> >   - Kill the calculator buffer after electric mode too.
> >   - Make decimal mode have "," groups, so it's more fitting for
> >     use in money calculations.
> >   - Factorial works with non-integer inputs.
> >   - Swallow less errors.
> >   - Lots of other improvements, but no changes to custom
> >     variables, or other user visible changes (except the above).
> 
> The above description is the kind of thing that should go into
> etc/NEWS.

Shouldn't the NEWS entry list only user-visible changes?  (That is,
only the first three items in the above.)  I also have a small bug fix
to commit, which also seems like something that shouldn't be mentioned
in the NEWS --?


> But you forgot to include a ChangeLog entry for that commit.

Should I add it now?  (I have a small bug fix in the new code, so
maybe I can add the previous text with this commit?)


> For the ChangeLog entry, you should instead include details about
> which parts of the code you changed (see
> https://www.gnu.org/prep/standards/html_node/Change-Logs.html).  You
> can use "C-x 4 a" (both from the calculator.el and the *vc-diff*
> buffers) to help you write that.

(AFAICT, it doesn't help with the actual log message, right?)


> And the commit message should be a textual copy of the ChangeLog
> entry just with the leading TABs stripped (you can use C-c C-a from
> the *vc-log* buffer to let Emacs fetch that ChangeLog text for you).

(I should obviously follow whatever is the convention, but as a
sidenote, this requirement is implicitly saying "I'm obsolete", since
if it's always followed, then it is identical to the commit log, and
therefore the changelog file can be auto-generated.  Similarly for the
log message contents -- which is specified rigidly to the point where
it can be done better with a VCS.  But I'll repeat to avoid flaming
followups: I will try to follow whatever the convention are.)

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Stefan Monnier]

> -          (cl-letf (((symbol-function 'F)
> -                     (lambda (&optional x y) (calculator-funcall f x y)))
> -                    ((symbol-function 'D)
> -                     (lambda (x) (if calculator-deg (/ (* x 180) float-pi) x))))
> -            (eval f `((X . ,X)
> -                      (Y . ,Y)
> -                      (TX . ,TX)
> -                      (TY . ,TY)
> -                      (DX . ,DX)
> -                      (L . ,L))))))
> -    (error 0)))
> +      (cl-flet ((F (&optional x y) (calculator-funcall f x y))
> +                (D (x) (if calculator-deg (/ (* x 180) float-pi) x)))
> +        (eval `(let ((X ,X) (Y ,Y) (DX ,DX) (TX ,TX) (TY ,TY) (L ',L))
> +                 ,f)
> +              t)))))

Hmm... have you tested this?  `cl-flet' creates lexically-scoped
function bindings (contrary to the old `flet'), so the F and D above in
your new code aren't accessible to the `f' expression.

As for changing the `eval' call, the form I used is more efficient than
the (eval `(let ...)) you're using now: what was the motivation for
this change?

>    (let ((inp (or keys (this-command-keys))))
[...]
> +      ;; translates kp-x to x and [tries to] create a string to lookup
> +      ;; operators; assume all symbols are translatable via
> +      ;; `function-key-map' or with an 'ascii-character property
> +      (concat (mapcar (lambda (k)
> +                        (if (numberp k) k (or (get k 'ascii-character)
> +                                              (error "??bad key??"))))
> +                      (or (lookup-key function-key-map inp) inp))))))

(this-command-keys) should return "fully decoded events", i.e. after
passing through the keyboard-coding-system, input-decode-map,
function-key-map, and key-translation-map.  So neither (lookup-key
function-key-map inp) nor (get k 'ascii-character) should be necessary.

IOW if this code really is needed, it deserves a comment explaining
why it's needed despite the fact that function-key-map was
already applied.


        Stefan

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Stefan Monnier]

>> >   * lisp/calculator.el: Lots of revisions
>> >   - Kill the calculator buffer after electric mode too.
>> >   - Make decimal mode have "," groups, so it's more fitting for
>> >     use in money calculations.
>> >   - Factorial works with non-integer inputs.
>> >   - Swallow less errors.
>> >   - Lots of other improvements, but no changes to custom
>> >     variables, or other user visible changes (except the above).
>> The above description is the kind of thing that should go into
>> etc/NEWS.
> Shouldn't the NEWS entry list only user-visible changes?  (That is,
> only the first three items in the above.)

That's right, I was mostly thinking of the first three (actually, the
first one is probably not worth mentioning).

>> But you forgot to include a ChangeLog entry for that commit.
> Should I add it now?

Yes, please.

>> For the ChangeLog entry, you should instead include details about
>> which parts of the code you changed (see
>> https://www.gnu.org/prep/standards/html_node/Change-Logs.html).  You
>> can use "C-x 4 a" (both from the calculator.el and the *vc-diff*
>> buffers) to help you write that.
> (AFAICT, it doesn't help with the actual log message, right?)

It's not intelligent, no, but it does write the file name and function
name, using the proper syntax.

> therefore the changelog file can be auto-generated.

That's the end goal, yes.


        Stefan

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Eli Barzilay]

On Wednesday, Stefan Monnier wrote:
> > -          (cl-letf (((symbol-function 'F)
> > -                     (lambda (&optional x y) (calculator-funcall f x y)))
> > -                    ((symbol-function 'D)
> > -                     (lambda (x) (if calculator-deg (/ (* x 180) float-pi) x))))
> > -            (eval f `((X . ,X)
> > -                      (Y . ,Y)
> > -                      (TX . ,TX)
> > -                      (TY . ,TY)
> > -                      (DX . ,DX)
> > -                      (L . ,L))))))
> > -    (error 0)))
> > +      (cl-flet ((F (&optional x y) (calculator-funcall f x y))
> > +                (D (x) (if calculator-deg (/ (* x 180) float-pi) x)))
> > +        (eval `(let ((X ,X) (Y ,Y) (DX ,DX) (TX ,TX) (TY ,TY) (L ',L))
> > +                 ,f)
> > +              t)))))
> 
> Hmm... have you tested this?  `cl-flet' creates lexically-scoped
> function bindings (contrary to the old `flet'), so the F and D above
> in your new code aren't accessible to the `f' expression.

[I know I did, since one of my commits fixed the `fib' example in the
`calculator-user-operators' docstring.  But I probably tested it with
24.3.1 which is my regular emacs, though something else must have
happened since it's broken there too now.]

Is there some way to make it work with 24.3.1 and the trunk version?
Using the original seems to complain about unbound functions when letf
is trying to save the old `symbol-function' values.


> As for changing the `eval' call, the form I used is more efficient
> than the (eval `(let ...)) you're using now: what was the motivation
> for this change?

The change I did makes it work in 24.3.1 (my regular version) which
doesn't have the new eval feature.  Efficiency is really not important
in this case, but making me test it more is important...


> >    (let ((inp (or keys (this-command-keys))))
> [...]
> > +      ;; translates kp-x to x and [tries to] create a string to lookup
> > +      ;; operators; assume all symbols are translatable via
> > +      ;; `function-key-map' or with an 'ascii-character property
> > +      (concat (mapcar (lambda (k)
> > +                        (if (numberp k) k (or (get k 'ascii-character)
> > +                                              (error "??bad key??"))))
> > +                      (or (lookup-key function-key-map inp) inp))))))
> 
> (this-command-keys) should return "fully decoded events", i.e. after
> passing through the keyboard-coding-system, input-decode-map,
> function-key-map, and key-translation-map.  So neither (lookup-key
> function-key-map inp) nor (get k 'ascii-character) should be
> necessary.
> 
> IOW if this code really is needed, it deserves a comment explaining
> why it's needed despite the fact that function-key-map was already
> applied.

It's code that has been there before, and I was never sure about it.
Here's what leads to it:

* If there is a binding for kp-whatever, rebinding whatever doesn't
  affect kp-whatever.

* Since I have (or had) a few of these, I made the code bind *both*
  kp-whatever and whatever.  (Thinking that for anyone, a good reason
  to have kp-whatever bound globally doesn't contradict calculator
  re-grabbing it.)

* With both bound, the lookup is needed, otherwise it ends up with
  [kp-7] and [kp-add] instead of "7" and "+".

The bottom line is, I'm not sure about the whole thing -- one of these
should be done:

1. The above is what it is, and needs to be synthesized into the
   comment you mention.  (Something that says that it's needed because
   kp-whatever key bindings are made.)

2. Maybe the kp-whatever key bindings should go away instead?  (I
   think that I don't need them anymore, but I hesitate to do such a
   big change in an area I don't know much about.)

3. It would really be best if there's a good way to just eliminate the
   need for the whole thing -- currently things like [kp-]7 and [kp-]+
   are bound to `calculator-op' and `calculator-digit', which need to
   lookup the actual key.  But I suppose that this will take time.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Eli Barzilay]

I ended up hacking `cl-flet' into the `eval'ed code, added the comment
about the key translation, fixed the recent bug, and committed this
with a changelog entry for the earlier commit too.

The only thing I didn't do is add a NEWS entry -- I'm not sure which
section of the file to add it to.  Should it be in the "Changes in
Specialized Modes and Packages in Emacs 24.5" section, as a new "**
Calculator" entry between "** Tramp" and "** Obsolete packages"?

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Nicolas Semrau]

Quick suggestion for further cleaning: Since calculator.el has been
part of Emacs for so long now, isn't the comment on lines 30--34
redundant nowadays (at least the part about autoloading)?

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Stefan Monnier]

> Is there some way to make it work with 24.3.1 and the trunk version?

Ah, so that's what it was about.

> Using the original seems to complain about unbound functions when letf
> is trying to save the old `symbol-function' values.

I guess you could use

    (eval `(cl-flet ...) t)
   
>> As for changing the `eval' call, the form I used is more efficient
>> than the (eval `(let ...)) you're using now: what was the motivation
>> for this change?
> The change I did makes it work in 24.3.1 (my regular version) which
> doesn't have the new eval feature.  Efficiency is really not important
> in this case, but making me test it more is important...

Since calculator.el is not distributed separately, I assumed there was
no need to preserve backward compatibility.

> * If there is a binding for kp-whatever, rebinding whatever doesn't
>   affect kp-whatever.
> * Since I have (or had) a few of these, I made the code bind *both*
>   kp-whatever and whatever.  (Thinking that for anyone, a good reason
>   to have kp-whatever bound globally doesn't contradict calculator
>   re-grabbing it.)
> * With both bound, the lookup is needed, otherwise it ends up with
>   [kp-7] and [kp-add] instead of "7" and "+".

Ah, right.  That makes a lot of sense.  I think you can drop the
ascii-character part because the function-key-map should cover a strict
superset, but indeed the function-key-map is still needed.

> 1. The above is what it is, and needs to be synthesized into the
>    comment you mention.  (Something that says that it's needed because
>    kp-whatever key bindings are made.)
> 2. Maybe the kp-whatever key bindings should go away instead?  (I
>    think that I don't need them anymore, but I hesitate to do such a
>    big change in an area I don't know much about.)
> 3. It would really be best if there's a good way to just eliminate the
>    need for the whole thing -- currently things like [kp-]7 and [kp-]+
>    are bound to `calculator-op' and `calculator-digit', which need to
>    lookup the actual key.  But I suppose that this will take time.

To the extent you want to override a global binding to kp-7, I think
number 1 is the right solution.


        Stefan

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Stefan Monnier]

> The only thing I didn't do is add a NEWS entry -- I'm not sure which
> section of the file to add it to.  Should it be in the "Changes in
> Specialized Modes and Packages in Emacs 24.5" section, as a new "**
> Calculator" entry between "** Tramp" and "** Obsolete packages"?

Sounds like a good place, yes.


        Stefan

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Eli Barzilay]

Yesterday, Stefan Monnier wrote:
> 
> > Using the original seems to complain about unbound functions when
> > letf is trying to save the old `symbol-function' values.
> 
> I guess you could use
> 
>     (eval `(cl-flet ...) t)

Yeah, that's what I ended up doing.


> Since calculator.el is not distributed separately, I assumed there
> was no need to preserve backward compatibility.

I'm not worried about backward compatibility in general -- just
locally, to the point where it works on the Emacs version I'm actually
using every day.


> Ah, right.  That makes a lot of sense.  I think you can drop the
> ascii-character part because the function-key-map should cover a
> strict superset, but indeed the function-key-map is still needed.

OK.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: [Emacs-diffs] trunk r117340: * lisp/calculator.el: Lots of revisions

[Stephen J. Turnbull]

Stefan Monnier writes:

 > Since calculator.el is not distributed separately, I assumed there was
 > no need to preserve backward compatibility.

Not by Emacs.  XEmacs, however, would appreciate the favor.