openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

As far as I know, you need to use the "deepcopy" method to make a deep copy.

Also, please write some tests for your db methods. If you had done so already, you would have picked up the bug I see.

Diff comments:

> 
> === added file 'openlp/core/utils/db.py'
> --- openlp/core/utils/db.py	1970-01-01 00:00:00 +0000
> +++ openlp/core/utils/db.py	2016-01-05 08:08:02 +0000
> @@ -0,0 +1,73 @@
> +# -*- coding: utf-8 -*-
> +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
> +
> +###############################################################################
> +# OpenLP - Open Source Lyrics Projection                                      #
> +# --------------------------------------------------------------------------- #
> +# Copyright (c) 2008-2016 OpenLP Developers                                   #
> +# --------------------------------------------------------------------------- #
> +# This program is free software; you can redistribute it and/or modify it     #
> +# under the terms of the GNU General Public License as published by the Free  #
> +# Software Foundation; version 2 of the License.                              #
> +#                                                                             #
> +# This program is distributed in the hope that it will be useful, but WITHOUT #
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or       #
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for    #
> +# more details.                                                               #
> +#                                                                             #
> +# You should have received a copy of the GNU General Public License along     #
> +# with this program; if not, write to the Free Software Foundation, Inc., 59  #
> +# Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
> +###############################################################################
> +"""
> +The :mod:`db` module provides helper functions for database related methods.
> +"""
> +import logging
> +
> +log = logging.getLogger(__name__)
> +
> +import sqlalchemy
> +
> +
> +def drop_columns(op, tablename, columns):
> +    """
> +    Column dropping functionality for SQLite, as there is no native support, neither in Alembic, nor in SQLite
> +
> +    From https://github.com/klugjohannes/alembic-sqlite
> +    """
> +
> +    # we need copy to make a deep copy of the column attributes
> +    from copy import copy

Surely if you want to make a true deep copy, you need to use deepcopy, not copy?

> +
> +    # get the db engine and reflect database tables
> +    engine = op.get_bind()
> +    meta = sqlalchemy.MetaData(bind=engine)
> +    meta.reflect()
> +
> +    # create a select statement from the old table
> +    old_table = meta.tables[tablename]
> +    select = sqlalchemy.sql.select([c for c in old_table.c if c.name not in columns])
> +
> +    # get remaining columns without table attribute attached
> +    remaining_columns = [copy(c) for c in old_table.columns if c.name not in columns]
> +    for column in remaining_columns:
> +        column.table = None
> +
> +    # create a temporary new table
> +    new_tablename = '{0}_new'.format(tablename)
> +    op.create_table(new_tablename, *remaining_columns)
> +    meta.reflect()
> +    new_table = meta.tables[new_tablename]
> +
> +    # copy data from old table
> +    insert = sqlalchemy.sql.insert(new_table).from_select([c.name for c in remaining_columns], select)
> +    engine.execute(insert)
> +
> +    # drop the old table and rename the new table to take the old tables
> +    # position
> +    op.drop_table(tablename)
> +    op.rename_table(new_tablename, tablename)
> +
> +
> +def drop_column(tablename, columnname):
> +    drop_column(tablename, [columnname])

I think you're using the wrong function here, it should be drop_columnS



-- 
https://code.launchpad.net/~sam92/openlp/multiple-songbooks/+merge/281582
Your team OpenLP Core is subscribed to branch lp:openlp.