It would be nice to have initargs defined for protobuf classes

[eschulte]

Is there a reason this isn't done currently? Thanks

[brown]

No good reason I think. There might be some trickiness with tracking which fields of the protobuf have been set.

[brown]

I had some time this weekend to experiment with your initarg patch. Thanks very much for sending it.

Figuring out what initarg handling should look like is important. A proto3 implementation should have initargs support, for instance, and it would be excellent for the initarg APIs for proto2 and proto3 to be compatible.

I found initargs to be hard to use in the presence of repeated fields. For instance, here's the existing code in the address book example to add a PERSON:

(defun add-person (&key id name email-address phone-numbers)
  (let ((address-book (read-address-book))
        (person (make-instance 'person)))
    (setf (id person) id)
    (setf (name person) (pb:string-field name))
    (when email-address
      (setf (email person) (pb:string-field email-address)))
    (loop for (type . number) in phone-numbers do
      (let ((phone-number (make-instance 'person-phone-number))
            (type (ecase type
                    (mobile +person-phone-type-mobile+)
                    (home +person-phone-type-home+)
                    (work +person-phone-type-work+))))
        (setf (number phone-number) (pb:string-field number))
        (setf (type phone-number) type)
        (vector-push-extend phone-number (phone person))))
    (vector-push-extend person (person address-book))
    (write-address-book address-book))
  (values))

Here's the same function but using initargs:

(defun add-person (&key id name email-address phone-numbers)
  (let* ((address-book (read-address-book))
         (phone (loop for (type . number) in phone-numbers
                      collect (make-instance 'person-phone-number
                                             :number (pb:string-field number)
                                             :type (ecase type
                                                     (mobile +person-phone-type-mobile+)
                                                     (home +person-phone-type-home+)
                                                     (work +person-phone-type-work+)))
                      into phones
                      finally (return (cl:make-array (length phones)
                                                     :initial-contents phones
                                                     :element-type 'person-phone-number
                                                     :adjustable t
                                                     :fill-pointer t))))
         (person (make-instance 'person
                                :id id
                                :name (pb:string-field name)
                                :phone phone)))
    (when email-address
      (setf (email person) (pb:string-field email-address)))
    (vector-push-extend person (person address-book))
    (write-address-book address-book))
  (values))

The need to initialize a repeated field with an adjustable array makes the code much longer.

Perhaps lists should be allowed as initarg arguments for repeated fields. I have to think more about how the design affects how proto3 support is implemented.

[eschulte]

I see your point, although I think there are other options for the above function in which initargs seems more readable (subjectively to my eye). E.g.,

(defun add-person (&key id name email-address phone-numbers
                        &aux (address-book (read-address-book)))
  (vector-push-extend
   (apply #'make-instance 'person
          :id id
          :name (pb:string-field name)
          :phone (map 'vector (lambda (pair)
                                (destructuring-bind (type . number)
                                    (make-instance 'person-phone-number
                                      :number (pb:string-field number)
                                      :type (ecase type
                                              (mobile +person-phone-type-mobile+)
                                              (home +person-phone-type-home+)
                                              (work +person-phone-type-work+)))))
                      phone-numbers)
          (when email-address (list :email (pb:string-field email-address))))
   (person address-book))
  (write-address-book address-book)
  (values)) 

Although the above gives a vector, it is not extendable and might not satisfy the type checks on the :phone slot. I might like the idea of allowing lists to be used for repeated fields instead of vectors--at least for initialization and setf although probably not for storage. This should be doable by customizing make-instance, e.g. (if we know that's the only time we have list fields we could do):

(defmethod make-instance
    ((class (eql 'example)) &rest options &key &allow-other-keys)
  (apply #'call-next-method class
         (mapcar (lambda (opt)
                   (typecase opt
                     (list (make-array (length opt)
                                       :initial-contents opt
                                       :adjustable t
                                       :fill-pointer t))
                     (t opt)))
                 options)))

If this seems reasonable (I can see an argument that it is too fancy/unexpected) I could put together a patch to this effect.

[eschulte]

Ping. Any thoughts on this?

[brown]

Yes, sorry for not responding. I'll send a more substantive note in a bit.

…

On Thu, Mar 19, 2020 at 3:42 PM Eric Schulte ***@***.***> wrote: Ping. Any thoughts on this? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#16 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGVMJ45CCW5BIZJ6ZHR6DRIJYR7ANCNFSM4KCSLLBQ> .