Can we change col-type checking to another place?

[daninus14]

Discussed in #154

^{Originally posted by daninus14 October 31, 2024}
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?

[daninus14]

I added a PR here: #155

It looks like one of the tests failed here https://github.com/fukamachi/mito/actions/runs/11612951970/job/32337587093#step:3:2828

Any ideas why?