Re: [PEPr] Comment on Tools and Utilities::DbDeploy

Alan, thanks for all the pointers. I knew the code wasn't up to standards, but wasn't exactly sure which the worst-offending parts were. I'll get started on cleaning that all up. somewhat related, is there a standard for version numbers of the package? right now I just called it 1.0.0, but am open to changing it to 0.x if there are certain 1.0-type standards to be met. as for the MDB2, I couldn't find a way to do what I want with it. this timestamp is more about SQL syntax/dialect generation because the deployment and/or rollback scripts are run independently of the dbdeploy command itself and the db itself needs to generate the timestamp, so it has to be done within the SQL. I didn't see any SQL syntax/dialect type features in MDB2. but I'll work to clean up my own SyntaxFactory stuff to make it as simple as possible. thanks again, -L On 17 Sep 2007 23:42:27 -0000, Alan Knowles <alan@akbkhome.com> wrote: > > > 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 >