Can we change col-type checking to another place?

[daninus14]

Hi,

In the code for initialize-instance here (copied below for convenience):

mito/src/core/class/column.lisp

Line 73 in 9a2f926

(unless (slot-boundp class 'col-type)

(defmethod initialize-instance :around ((class table-column-class) &rest rest-initargs
                                        &key name initargs ghost
                                        &allow-other-keys)
  (declare (ignore ghost))
  (unless (find (symbol-name name) initargs :test #'string=)
    ;; Add the default initarg.
    (push (intern (symbol-name name) :keyword)
          (getf rest-initargs :initargs)))

  (let ((class (apply #'call-next-method class rest-initargs)))
    (unless (slot-boundp class 'col-type)
      (if (or (ghost-slot-p class)
              (slot-value class 'references))
          (setf (slot-value class 'col-type) nil)
          (error 'col-type-required
                 :slot class)))
    class))

A check for col-type's presence is done by this code:

(unless (slot-boundp class 'col-type)
      (if (or (ghost-slot-p class)
              (slot-value class 'references))
          (setf (slot-value class 'col-type) nil)
          (error 'col-type-required
                 :slot class)))

This is causing issues for me with compute-effective-slot-definition here https://github.com/sbcl/sbcl/blob/master/src/pcl/std-class.lisp#L1278

Here's the code for reference:

(defmethod compute-effective-slot-definition ((class slot-class) name dslotds)
  (let* ((initargs (compute-effective-slot-definition-initargs class dslotds))
         (class (apply #'effective-slot-definition-class class initargs))
         (slotd (apply #'make-instance class initargs)))
    slotd))

Because before I can setf the col-type myself in my own compute-effective-slot-definition method, it's signaling an error.

Why is the error signaled on initialize-instance? Could we change that to somewhere else like whenever generating the SQL where it may make more sense? When we generate the SQL we need col-type to be defined. However at the time of initialization, I don't think it's needed.

I think this is more sensible and allows for the project to be extensible. What do you think?

[daninus14]

From here it looks like it's used in only 7 source files: https://github.com/search?q=repo%3Afukamachi%2Fmito%20col-type&type=code

1. mito/src/core/dao/mixin.lisp

   Line 120 in 9a2f926

   for col-type = (table-column-type column)

2. mito/src/core/dao/mixin.lisp

   Line 120 in 9a2f926

   for col-type = (table-column-type column)

3. mito/src/core/dao/table.lisp

   Line 56 in 9a2f926

   append (let ((col-type (table-column-type column)))

4. mito/src/core/conversion.lisp

   Line 23 in 9a2f926

   (:method ((driver-type (eql :mysql)) (col-type (eql :boolean)) value)

5. mito/src/core/class/column.lisp

   Line 24 in 9a2f926

   (defun parse-col-type (col-type)

6. mito/src/core/class/table.lisp

   Line 48 in 9a2f926

   (loop for column in (getf initargs :direct-slots)

7. mito/src/core/dao/column.lisp

   Line 91 in 9a2f926

   (:method ((col-type (eql :timestamptz)) value)

Every single one that could be problematic calls (table-column-type column) or some other accessor. Calling an accessor on an unbound slot will give an error anyways already. I think the benefit of the current code in

mito/src/core/class/column.lisp

Line 73 in 9a2f926

(unless (slot-boundp class 'col-type)

is to help during development of mito, but functionality wise, the code will anyways produce an error if col-type is unbound when it shouldn't.

Therefore I think removing this error signaling should not actually affect the functionality of mito in any way, and it's safe to remove.

What do you think? Do you agree? Would you accept a PR?

[fukamachi]

Thank you for your suggestion. It totally sounds great to me!
The error was just because I thought it would make a meaningful message to users.

[daninus14]

Ok, great! I added two PR's one for the col-type check here #155

And another one to fix the effective slot functionality in mito here #159