Refactoring

[zorglube]

• Handle AutoCloseable Connexion and Database
• Improved Resource loading

[zorglube]

@lbruun : here is a king of rebase ;-)

[lbruun]

Hi zorglube. Your PR includes too many unrelated changes (something not related to what you plan to fix). It makes it impossible to review.

If the aim is to

"

• Handle AutoCloseable Connexion and Database
• Improved Resource loading

"

Then do that .. and only that. I think you have accidentally set your IDE to re-format source code files when you open them .. or something.

[zorglube]

Hey @lbruun,

Yes IDE certainly have a formatter different than your's. I'll handle the formatting issue.

About the formatting, I'll make some updates on the PR about Formatting I have made previously.

See you ;-)

[zorglube]

@lbruun : I handled whitespaceschanges.

[lbruun]

@zorglube . You are still re-formatting all of the code to the way you have set your formatter. Don't touch formatting when contributing to FOSS projects. Just look at the convention used and go with that.

Also, I have a bunch of other comments on things I do not like in the PR. I've commented directly in a few places.

I don't want this to be too discouraging for you, but you are simply attempting to change too many things in a single PR. It therefore becomes impossible to review. I'm not able to "see the wood for the trees", so to speak. (l'arbre cache la forêt).

For this reason, I'll have to reject.

Your idea of taking advantage of the fact that the Liquibase Database and DatabaseConnection now implements Autocloseable - and that the code in the project's LiquibaseUtils class can therefore now be greatly simplified - is a good one and I'll implement that and credit you. Thanks.

Your other idea, changing the type of sqlScriptReferences from List<String> to List<Resource> in the PreLiquibaseProperties class, is still something I'm not convinced about. I would have to see it in a change which only does that in order to evaluate if it is a simplification of the code or not, or brings other benefits.

[lbruun]

See PR #20

[zorglube]

Your other idea, changing the type of sqlScriptReferences from List<String> to List<Resource> in the PreLiquibaseProperties class, is still something I'm not convinced about. I would have to see it in a change which only does that in order to evaluate if it is a simplification of the code or not, or brings other benefits.

@lbruun Using the Spring Resource simplify a lot the resource loading. Using Spring Resource allow you to provide file, http, classpath and so on file URI without having to handle the the difference by hand. Using Resource allow to have script = classpath:/foo/bar/file.sql or script = file:/foo/bar/file.sql even script = http://uri.tld/foo/bar/file.sql.

Like it's written in the Spring doc : https://docs.spring.io/spring-framework/docs/6.0.x/reference/html/core.html#resources

Since Pre-Liquibase is designed to be a Spring module, why not using some powerful behavior that come built-in in Spring.

[zorglube]

@zorglube . You are still re-formatting all of the code to the way you have set your formatter. Don't touch formatting when contributing to FOSS projects. Just look at the convention used and go with that.

@lbruun I purposed a PR that contain an Eclipse formatter for the project, if might not be perfect today. If you have a formatter, or if you can extract your formatter configuration, it can help to end the related problems.

[zorglube]

I don't want this to be too discouraging for you, but you are simply attempting to change too many things in a single PR. It therefore becomes impossible to review. I'm not able to "see the wood for the trees", so to speak. (l'arbre cache la forêt).

I understand, I'll have a look and maybe purpose some other PR.

[lbruun]

Your other idea, changing the type of sqlScriptReferences from List<String> to List<Resource> in the PreLiquibaseProperties class, is still something I'm not convinced about. I would have to see it in a change which only does that in order to evaluate if it is a simplification of the code or not, or brings other benefits.

@lbruun Using the Spring Resource simplify a lot the resource loading. Using Spring Resource allow you to provide file, http, classpath and so on file URI without having to handle the the difference by hand. Using Resource allow to have script = classpath:/foo/bar/file.sql or script = file:/foo/bar/file.sql even script = http://uri.tld/foo/bar/file.sql.

Like it's written in the Spring doc : https://docs.spring.io/spring-framework/docs/6.0.x/reference/html/core.html#resources

But this is already the case. It is just initially represented as a String. All resource abstractions that Spring supports are supported, by definition. None of these Spring Resource abstractions are handled "by hand".

As indicated the code originally comes from Spring 2.3, class DataSourceInitializer, so at least some point in time someone from Spring Boot project decided that this was a good way to do this. This is not to say that the Spring folks are always right. I can also see that they've reorganized later but it looks to me as if they are are still using List<String> even in Spring Boot 3.0.