Repository: apache/superset
File: superset/daos/dataset.py (Python)

Code before:
            return False

    @classmethod
    def update(
        cls,
        item: SqlaTable | None = None,
        attributes: dict[str, Any] | None = None,
    ) -> SqlaTable:
        """
        Updates a Dataset model on the metadata DB
        """

        if item and attributes:
            force_update: bool = False
            if "columns" in attributes:
                cls.update_columns(
                    item,
                    attributes.pop("columns"),
                    override_columns=bool(attributes.get("override_columns")),
                )
                force_update = True

            if "metrics" in attributes:
                cls.update_metrics(item, attributes.pop("metrics"))
                force_update = True

            # columns and metrics are stored in separate tables so if they change we
            # force an update to the dataset too to ensure the last_updated_timestamp
            # is updated (it's possible no other dataset attributes are changed).
            if force_update:
                db.session.execute(update(SqlaTable).where(SqlaTable.id == item.id))

        return super().update(item, attributes)

    @classmethod
    def update_columns(
        cls,
        model: SqlaTable,
        property_columns: list[dict[str, Any]],
        override_columns: bool = False,
    ) -> None:
        """
        Creates/updates and/or deletes a list of columns, based on a
        list of Dict.

        - If a column Dict has an `id` property then we update.
        - If a column Dict does not have an `id` then we create a new metric.
        - If there are extra columns on the metadata db that are not defined on the List
        then we delete.
        """


Review comment (suggestion):
@Vitor-Avila Why not be explicit about this operation and update the `changed_on` field on the dataset? It would avoid the long comment and the cognitive load to understand that `changed_on` will be automatically updated once the record is changed.

Code after:

    @classmethod
    def update(
        cls,
        item: SqlaTable | None = None,
        attributes: dict[str, Any] | None = None,
    ) -> SqlaTable:
        """
        Updates a Dataset model on the metadata DB
        """

        if item and attributes:
            force_update: bool = False
            if "columns" in attributes:
                cls.update_columns(
                    item,
                    attributes.pop("columns"),
                    override_columns=bool(attributes.get("override_columns")),
                )
                force_update = True

            if "metrics" in attributes:
                cls.update_metrics(item, attributes.pop("metrics"))
                force_update = True

            if force_update:
                attributes["changed_on"] = datetime.now(tz=timezone.utc)

        return super().update(item, attributes)

    @classmethod
    def update_columns(
        cls,
        model: SqlaTable,
        property_columns: list[dict[str, Any]],
        override_columns: bool = False,
    ) -> None:
        """
        Creates/updates and/or deletes a list of columns, based on a
        list of Dict.

        - If a column Dict has an `id` property then we update.
        - If a column Dict does not have an `id` then we create a new metric.
        - If there are extra columns on the metadata db that are not defined on the List
        then we delete.
        """

        for column in property_columns:
            if (
                "python_date_format" in column
                and column["python_date_format"] is not None