These were the questions and comments I had from going through all the commits. Once we get them figured out we can finish the merge into liquibase master. NOTE: I’m going to be away from my computer for the next couple days.
You made some changes to how the logging works, introducing TreeStyleLogger and adding an SQL level. You also made some changes to what goes to STDERR and STDOUT. They are good changes, but one thing I’ve had on the list for Liquibase 3.6 has been a switch to slf4j (I used it in Liquibase 4).
If we use a mores standard logger, the sql level is probably not something we can add, but I do want the logger to be able to tell SQL statements from non-sql statements. I think there are ways in slf4j that we can do that, but haven’t gotten a chance to research the best options yet.
I didn’t look at your TreeStyleLogger output, but wondering if the MDC data in SLF4j would be capturing what the tree-style is adding while still being cross-logger-compatible? If TreeStyleLogger is more about formatting the output, will it still work with SLF4j?
One thing the current Liquibase use of stderr and stdout does is make sure that if you are running updateSql you can pipe that directly into your sql prompt, unix-style. I’m thinking your switching with stderr and stdout may have broken that functionality. Did it?
The way logging vs. command output works in general needs to support that pipe use case as well. Part of the switch in the logging system was going to be a better distinction between command-line output vs. logging.
Except for needing to look harder at how stderr and stdout work for pipe, I’d be OK merging the logging changes into master, but would like to make the switch to slf4j in 3.6 so what is in there would be refactored.
Somewhat logging-related is the adding of the banner in the command line output. It can be helpful, but also gets in the way of pipe-ing output so I’d tend to say it’s not worth adding. Adding the build info to the help docs or a separate command would be useful, though.
Also somewhat logging related, 7e30edd took out catching Throwable, but there are places where Throwables as thrown in jdbc drivers. etc and we like to catch that to provide better context around the exception.
Line Ending Standardization
There were a couple changes you put in around handling of line endings. There was a bug that I’ve fixed recently but may not have made it into the main repo yet that may also fix some of that. I’ll see if that change also addresses some of the changes you did. I think all the checksuming should be line-ending independent.
Database Version Support
You cut off support for SQL Server prior to 2008 which I think is fine, that is very old.
However, You took out support for for pre-9.2 postgres but I didn’t see a lot of great reasons to need to. That is still fairly old, but probably not so old that we can drop support for it unless there is a great reason. Why were you thinking it should be dropped?
While I like the idea of localizing Liquibase, when I’ve tried in the past I run into problems keeping everything in sync. When I make a change or get a pull request with a change, just the english version would be updated and I need to wait on non-english speakers to be able to catch things up which can be difficult to impossible and in the end the non-english documentation and prompts were out date which tends to be as bad as non-localized.
However, that’s my view as an english-native speaker How important and useful do you feel localization is vs. the overhead of maintaining it?
a86a4345 added logic that better handles the changeSet’s context expressions in the databasechangelog table, but it duplicates the logic a bit. I think the logic should actually just go in the ContextExpression.toString() method
6675e15 removed the shouldAddPrimaryKey method, but that is used by extensions and so needs to stay
23809c1d switches includeCatalog, includeSchema, and includeTablespace to be true by default which will cause problems for people expecting the old version and so need to stay the old way
c4b9a9c7 makes the Loader not skip classes with $ signs in them, but I’d be worried that people may have their own extensions which include inner-class defined services which would have a $ in the name. Note: for Liquibase 4 I’m using the Java ServiceLoader support and will probably backport that in 3.7.
7d79ee72 removed the canCacheLiquibaseInfo method, but I believe that is also used by extensions and so needs to stay
5fbec083 removed Database.correctSchema which is a deprecated method, but removing the method will also break extensions with @Override so I think it’s better to just leave in for now
Why did you take out the null check in 1e32c349?
613r6s30 takes out bracketing of datatypes in mssql, but I seem to remember that there was a pull request that explicitly had them added because they should always be bracketed. I’ll have to see if I can find why that was.