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

I made just about all the suggested changes. The package is now DB_Deploy and the directories and classes are changed to match PEAR conventions. I also changed the code formatting as indicated, and tidied up the SQL concat lines. I'm not sure how to add a link to the svn on the proposal ... I don't see an "Edit" button anymore. so here's a link to the svn @ SF: http://peardbdeploy.svn.sourceforge.net/viewvc/peardbdeploy/ -L On 9/17/07, Luke Crouch <luke.crouch@gmail.com> wrote: > > 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éå > > ¶‡m …Ih%þ > > > >