CORE-880 would be a good one to start with. I think the best thing to do is if they put --logLevel or some other unexpected param after the command, it outputs an “unexpected command parameter X” and the help documentation, perhaps with a sentence or two explaining that parameters after the command apply to the command, whereas parameters before the command apply to liquibase setup.
Maven and Git are not to bad to get used to. I actually don’t use maven much myself, I use intellij and have the project configuration committed to git so you just have to open the project and it compiles with the normal intellij compile.
If you use a different editor, it may be easiest to install maven (simply download from apache and unzip, then run mvn package from the root of the liquibase source tree.
Git is also relatively straightforward as well. http://learn.github.com/p/normal.html gives a starting tutorial, and you can find many others on the web. Many IDEs have built-in support for git, so it acts much like SVN, but with an extra “push” command that pushes the changes to the remote repository rather than just your local repository. You can either fork the liquibase github repository, or send patches. Whichever you prefer.
I really like what I’ve seen so far and would like to start contributing to the project, as time allows. I figured I would start slow and start learning the code base. I reviewed JIRA and started looking at some of the defects.
The bad part is 1) I am new to Maven and 2) new to GIT.
I took a look at defect CORE-880 and I see the issue reported. Based on the code this looks like more of documentation situation rather than a defect. Looks like once you find the ‘command’ then all the parameters processed after the command are considered parameters to the command argument, in this case it was update. If the user puts the --logLevel=debug before the ‘command’ then the parsing will set the logLevel field and the user would get their debug logging. Not sure whether you wanted to add code to double-check the command parms to see if they could actually be for something like this case or just provide some more documentation that explains the command line format that is expected. For example mentioning that once the code finds the command, then all parameters after that are passed along to the command processing rather than setting the local fields, like logLevel?
Based on your response, sounds like we want to allow the command to continue but just log this extra ‘warning’ about an unexpected parameter(s). After doing some digging, it looks like this would need to be done in a couple different places since not all command executions actually pass the commandParam set. For example, there’s a large set of no-arg commands, like update, updateSQL, futureRollbackSQL, generateChangeLog, validate, dropAll, releaseLocks (and more). For those, I would propose adding a new method similar to the checkSetup() that checks to see if the command is a ‘no-arg’ command and if true and the commandParams is not empty then we could log those there.
Commands like diff and diffChangeLog use the createReferenceDatabaseFromCommandParams() method and it looks like a simple else if at the end would allow us to log the ‘unexpected’ params.
Other command like tag and dbdoc are worse off since they simply call commandParams.iterator().next() and use the returned value for whatever parameter/value that command is expecting.
Does this match what you were thinking, or would you prefer a different approach?
Okay good - then I might be able to piggyback those checks inside of Main.checkSetup() since that will give the desired termination of the execution.
Next quick question - trying to get my head around the whole test process. I would like to create a set of testcases first that ‘fail’ and then fix the code and watch the testcases pass. I will search thru the forums because I seem to remember seeing a thread that mentioned test setup, etc.
Do you know if testcases for something like already exists?
What things/issues/etc should I be aware of in trying to setup test cases to cover this fix?
Usually the unit test classes are just the name of the class with Test appended. Same directory structure, but in /test/ instead of /main/. There is an existing MainTest class you should be able to look at and add cases as needed.
Improving the test cases is defintiely good, thanks!
I think it would be best to not just output a warning and continue execution, but output the warning and stop execution. If params are in the wrong spot, the command may do something completely different than what was expected and they may not see the warning.
I think your implementation ideas seem good. For tag and dbdoc and other commands that take a string, not params, you should be able to do a commandParams.size()==0 && !commandParams.iterator().next().startsWith("-") check I think.
I’ve got the code and test cases done and tested. Just want to do a bit more testing, hopefully tonight. Next up is reading the GIT tutorial so I can figure out how to get the code back into github and available to you. Once I commit the code changes, I will update the Wiki page(s) to mention the new ‘unexpected parameter’ checks that are added.
