Re: [elpa] externals/transient 667ce2b287 18/23: Use transient-default-value in transient-init-value(suffix)

Re: [elpa] externals/transient 667ce2b287 18/23: Use transient-default-value in transient-init-value(suffix)

[Stefan Monnier]

Hi Jonas,

> --- a/lisp/transient.el
> +++ b/lisp/transient.el
> @@ -3331,9 +3331,13 @@ Use `transient-default-value' to determine the default value."
>                (cdr saved)
>              (transient-default-value obj)))))
>  
> -(cl-defmethod transient-init-value ((_   transient-suffix))
> -  "Non-infix suffixes usually don't have a value, so this is a noop."
> -  nil)
> +(cl-defmethod transient-init-value ((obj transient-suffix))
> +  "Non-infix suffixes usually don't have a value.
> +Call `transient-default-value' but because that is a noop for
> +`transient-suffix', this function is effectively also a noop."
> +  (let ((value (transient-default-value obj)))
> +    (unless (eq value eieio--unbound)
> +      (oset obj value value))))
>  
>  (cl-defmethod transient-init-value ((obj transient-argument))
>    "Extract OBJ's value from the value of the prefix object."
> @@ -3379,6 +3383,11 @@ that.  If the slot is unbound, return nil."
>          default)
>      nil))
>  
> +(cl-defmethod transient-default-value ((_   transient-suffix))
> +  "Return `eieio--unbound' to indicate that there is no default value.
> +Doing so causes `transient-init-value' to skip setting the `value' slot."
> +  eieio--unbound)
> +
>  ;;;; Read
>  
>  (cl-defgeneric transient-infix-read (obj)

Why use `eieio--unbound` (i.e. a variable internal to EIEIO) rather than
your own magic placeholder?
IOW, is there a good reason to break the abstraction here?


        Stefan

Re: [elpa] externals/transient 667ce2b287 18/23: Use transient-default-value in transient-init-value(suffix)

[Jonas Bernoulli]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hi Jonas,
>
>> --- a/lisp/transient.el
>> +++ b/lisp/transient.el
>> @@ -3331,9 +3331,13 @@ Use `transient-default-value' to determine the default value."
>>                (cdr saved)
>>              (transient-default-value obj)))))
>>  
>> -(cl-defmethod transient-init-value ((_   transient-suffix))
>> -  "Non-infix suffixes usually don't have a value, so this is a noop."
>> -  nil)
>> +(cl-defmethod transient-init-value ((obj transient-suffix))
>> +  "Non-infix suffixes usually don't have a value.
>> +Call `transient-default-value' but because that is a noop for
>> +`transient-suffix', this function is effectively also a noop."
>> +  (let ((value (transient-default-value obj)))
>> +    (unless (eq value eieio--unbound)
>> +      (oset obj value value))))
>>  
>>  (cl-defmethod transient-init-value ((obj transient-argument))
>>    "Extract OBJ's value from the value of the prefix object."
>> @@ -3379,6 +3383,11 @@ that.  If the slot is unbound, return nil."
>>          default)
>>      nil))
>>  
>> +(cl-defmethod transient-default-value ((_   transient-suffix))
>> +  "Return `eieio--unbound' to indicate that there is no default value.
>> +Doing so causes `transient-init-value' to skip setting the `value' slot."
>> +  eieio--unbound)
>> +
>>  ;;;; Read
>>  
>>  (cl-defgeneric transient-infix-read (obj)
>
> Why use `eieio--unbound` (i.e. a variable internal to EIEIO) rather than
> your own magic placeholder?

I first used `eieio--unbound' "manually" (in another package), when it
was still named `eieio-unbound'.  When you made that breaking change,
I quickly added the necessary backward compatibility alias to that
package.  I have (in a later) commit done the same here.  (I briefly
forgot that this is not the package in which I had already been using
this symbol.  This may have contributed to me using this symbol here;
I though I was already using it.)

The reason I am using Eieio's "this is not bound", is that I am doing an
Eieio thing here.  It's not a "not bound" slot but the "not bound"
return value of a generic function, which isn't the same thing, but
pretty damn close.

Sure I can invent a new symbol just for this one case, but it would be
(IMO unnecessary) noise and the only reasons I would be doing it is to
make you happy and to avoid having a conversation about it.

> IOW, is there a good reason to break the abstraction here?

I am not sure what "the abstraction" and "here" refer to exactly.

Re: [elpa] externals/transient 667ce2b287 18/23: Use transient-default-value in transient-init-value(suffix)

[Stefan Monnier]

> The reason I am using Eieio's "this is not bound", is that I am doing an
> Eieio thing here.  It's not a "not bound" slot but the "not bound"
> return value of a generic function, which isn't the same thing, but
> pretty damn close.

🙂

> Sure I can invent a new symbol just for this one case, but it would be
> (IMO unnecessary) noise and the only reasons I would be doing it is to
> make you happy and to avoid having a conversation about it.

Sorry about this conversation.  I'm not bothered by your use of
`eieio--unbound`, to be honest, I was just curious.

[ If I were you, I think I'd just use something like `:unbound` or
  `:transient--unbound` because it's literally less code than what you
  currently have and it would save me from having to worry about the
  annoying guy breaking compatibility again.  ]

>> IOW, is there a good reason to break the abstraction here?
> I am not sure what "the abstraction" and "here" refer to exactly.

One of the abstractions is just that the `eieio--` prefix implies it's an
internal entity to EIEIO so shouldn't be used from outside of EIEIO's
own code.

The other is that `eieio--unbound` is a value that ideally noone should
ever see, just like `Qunbound` in Emacs's C code.  Every time you use it
you increase the risk that someone ends up storing it inside an
EIEIO object.
[ Now that I think about it... I'm no fan of EIEIO objects, especially
  that notion of a slot being unbound, so maybe I should encourage use
  of `eieio--unbound` so as to maximize the chance of mayhem.  🙂  ]


        Stefan

Re: [elpa] externals/transient 667ce2b287 18/23: Use transient-default-value in transient-init-value(suffix)

[Jonas Bernoulli]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The reason I am using Eieio's "this is not bound", is that I am doing an
>> Eieio thing here.  It's not a "not bound" slot but the "not bound"
>> return value of a generic function, which isn't the same thing, but
>> pretty damn close.
>
> 🙂
>
>> Sure I can invent a new symbol just for this one case, but it would be
>> (IMO unnecessary) noise and the only reasons I would be doing it is to
>> make you happy and to avoid having a conversation about it.

I think I'll leave it as-is.  Since the effect of this function
returning eieio--unbound is that the caller keeps using eieio--unbound
as the "not-a-value" marker of this slot, I think that using this symbol
is more appropriate than using any other symbol, which would obfuscate
this relationship.

> Sorry about this conversation.  I'm not bothered by your use of
> `eieio--unbound`, to be honest, I was just curious.
>
> [ If I were you, I think I'd just use something like `:unbound` or
>   `:transient--unbound` because it's literally less code than what you
>   currently have and it would save me from having to worry about the
>   annoying guy breaking compatibility again.  ]

Luckily I though about this remark long enough to realize that you were
not calling me "the annoying guy".  But wording that sentence like this
was a bit risky...

Anyway, sorry for the saltiness of my previous message.

>>> IOW, is there a good reason to break the abstraction here?
>> I am not sure what "the abstraction" and "here" refer to exactly.
>
> One of the abstractions is just that the `eieio--` prefix implies it's an
> internal entity to EIEIO so shouldn't be used from outside of EIEIO's
> own code.
>
> The other is that `eieio--unbound` is a value that ideally noone should
> ever see, just like `Qunbound` in Emacs's C code.  Every time you use it
> you increase the risk that someone ends up storing it inside an
> EIEIO object.

My other package which uses eieio--unbound is closql, whose purpose is
to store eieio object in a sqlite database.  Since Eieio support unbound
slots, I have no choice but to deal with them.

And in this package I indeed have to be super careful when dealing with
this symbol.  Especially because it crosses the Lisp/SQLite border at
which point the distinction whether eieio--unbound is interned or not is
lost.  (I.e., if someone actually stored an interned eieio--unbound in
an object, then we lose.)

> [ Now that I think about it... I'm no fan of EIEIO objects, especially
>   that notion of a slot being unbound, so maybe I should encourage use
>   of `eieio--unbound` so as to maximize the chance of mayhem.  🙂  ]

I expect that in 5 to 10 years you will present a new object system,
until then I'll stick to this one. :P

About the unbound thing, I have mixed feelings.  But we all start at the
beginning, i.e., when we know nothing about something and start learning
about it.  In the case of Eieio we learn (among many other things of
course) that there is such a thing as unbound slots, and if we have no
experience with other objects systems (which was the case for me), we
cannot help but assume that there is a good reason for that feature to
exist, and then move on to think about when to use it.  Over time we
learn about disadvantages of the feature and when not to use it.  By now
I initialize most of my slots with ":initform nil", but in a few cases
it's still useful, and I figure if it exists, I might as well use it
when that is the case.  But I probably would not argue for the "unbound
unless told otherwise" default, and if this did not exist at all, I
probably would not miss it.  But at this time removing this feature also
does not sound feasible.

     Happy holidays!
     Jonas

Re: [elpa] externals/transient 667ce2b287 18/23: Use transient-default-value in transient-init-value(suffix)

[Stefan Monnier]

> Luckily I though about this remark long enough to realize that you were
> not calling me "the annoying guy".  But wording that sentence like this
> was a bit risky...

🙂

> My other package which uses eieio--unbound is closql, whose purpose is
> to store eieio object in a sqlite database.  Since Eieio support unbound
> slots, I have no choice but to deal with them.

I see.

>> [ Now that I think about it... I'm no fan of EIEIO objects, especially
>>   that notion of a slot being unbound, so maybe I should encourage use
>>   of `eieio--unbound` so as to maximize the chance of mayhem.  🙂  ]
> I expect that in 5 to 10 years you will present a new object system,
> until then I'll stick to this one. :P

FWIW, I use `cl-defstruct` instead for now (which sucks in its own way).
But your expectation might not be too far off the mark.

> But at this time removing this feature also does not sound feasible.

+1


        Stefan