[PEPr] Comment on Tools and Utilities::DbDeploy

Alan Knowles (http://pear.php.net/user/alan_k) has commented on the proposal for Tools and Utilities::DbDeploy. Comment: Can you make the sources browse-able online. (just link to subversion on sourceforge) - DbDeploy is not really a PEAR compatible package name. DB_Deploy should be OK. The syntax packages should be something like: DB_Deploy_Syntax_MSSQL etc. and the directory structure should map with the _ => / Coding standard need following - 4 spaces, brace locating etc, short ifs.. The code could do with quite a bit of tidying up: eg. $sqlToPerformDeploy .= 'UPDATE ' . DbDeploy::$TABLE_NAME . ' SET complete_dt = ' . $this->dbmsSyntax->generateTimestamp() . ' WHERE change_number = ' . $fileChangeNumber . ' AND delta_set = \'' . $this->deltaSet . '\';' . "\n"; $sqlToPerformDeploy .= '--------------- Fragment ends: ' . $fileChangeNumber . ' ---------------' . "\n"; Would be considerably more readable as follows (and could still be improved) $sqlToPerformDeploy .= 'UPDATE ' . DbDeploy::$TABLE_NAME . ' SET complete_dt = ' . $this->dbmsSyntax->generateTimestamp() . ' WHERE change_number = ' . $fileChangeNumber . ' AND delta_set = \'' . $this->deltaSet . "';\n" . '--------------- Fragment ends: ' . $fileChangeNumber . ' ---------------' . "\n"; without understanding the code, does it really need lot's of set**** methods? - can that be simplified.. simpler API? the Abstract class DbmsSyntax appears to be rather pointless.. - Why not make the Syntax files extend the factory and throw an exception on that single method that is required. Factory class: case is a statement, not a function Looking at the Syntax drivers - can you see if MDB2 provides that ability - formating timestamp code... - rather than re-writing it..? Otherwise looks like a reasonable package to include. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=507 -- Sent by PEPr, the automatic proposal system at http://pear.php.net