Add missing #depends_on method to ProductLocation by dgdavid · Pull Request #462 · yast/yast-packager (original) (raw)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I see with the ProductLocation and ProductLocationDetails approach is that the details attribute could be nil. This happens because :product is an optional parameter in ProductLocaltion#initialize.

So, checking the existence of details is always mandatory: product.details&.attribute. And this has an implicit drawback (IMHO). The caller is the responsible to define the default value when details or the attribute are nil:

product.details&.attribute || []

For me, a better solution would be to force ProductLocation to always have a details object. And then I would define proper default values for each detail attribute:

class ProductLocation
  def initialize(name, dir, product: nil)
    @details = product || ProductLocationDetails.new
  end
end

class ProductLocaltionDetails
  def initialize(product: nil, depends_on: [], ...)
  end
end