Refactoring #19

zorglube · 2022-11-29T17:46:03Z

Handle AutoCloseable Connexion and Database
Improved Resource loading

- Handle AutoCloseable Connexion and Database - Improved Resource loading

zorglube · 2022-11-29T17:48:36Z

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

lbruun · 2022-11-29T20:23:55Z

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 · 2022-11-30T09:42:35Z

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 · 2022-12-02T11:31:56Z

@lbruun : I handled whitespaceschanges.

lbruun · 2022-12-04T18:24:10Z

autoconfigure/src/main/java/net/lbruun/springboot/preliquibase/PreLiquibaseProperties.java

-import jakarta.validation.constraints.NotEmpty;
-import org.springframework.boot.context.properties.ConfigurationProperties;
-import org.springframework.validation.annotation.Validated;
+import static java.lang.String.format;


Not a fan of these static imports. It makes the code less readable.

In my mind

LiquibaseUtils.getLiquibaseDatabaseShortName(...)

is a lot easier to read than

getLiquibaseDatabaseShortName(...)

because the former makes it obvious that the method is something from outside the class.

It's a matter of personal habits and preferences. Anyhow since it's your project, I'll comply.

lbruun · 2022-12-04T18:25:13Z

autoconfigure/src/main/java/net/lbruun/springboot/preliquibase/PreLiquibaseProperties.java


 /**
 * Properties for Pre-Liquibase module.
 *
 * @author lbruun
 */
-@Validated
-@ConfigurationProperties(prefix = PROPERTIES_PREFIX)
+@ConfigurationProperties(prefix = "preliquibase")


Why this change?

Because after my refactoring it has become useless to have the configuration key prefix stored into a static var.

lbruun · 2022-12-04T18:29:45Z

autoconfigure/src/main/java/net/lbruun/springboot/preliquibase/PreLiquibaseProperties.java

@@ -221,7 +259,41 @@ public List<String> getSqlScriptReferences() {
     *
     * @param sqlScriptReferences list of Spring Resource references.
     */
-    public void setSqlScriptReferences(List<String> sqlScriptReferences) {
+    // @formatter:on


What is this? No IDE specific stuff in the code, please.

It's an Eclipse specific comment. Like I written before, your project your rules.

lbruun · 2022-12-04T18:36:46Z

pom.xml

@@ -87,10 +85,14 @@ limitations under the License.
                <type>pom</type>
                <scope>import</scope>
            </dependency>
+            <dependency>


I see the point here. Don't really know if it improves anything.

In any case the elements are now misaligned and for this reason alone I'll have to reject it (regardless if the idea is a good one or not .. which I cannot quite decide upon yet)

lbruun · 2022-12-04T18:38:46Z

autoconfigure/src/main/java/net/lbruun/springboot/preliquibase/PreLiquibase.java

@@ -5,7 +5,7 @@
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
- *      http://www.apache.org/licenses/LICENSE-2.0


Ooops. Why ????
This is the text from the AL2 license. It must not be touched by an (accidental) re-format of code.

lbruun · 2022-12-04T19:19:03Z

@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 · 2022-12-04T19:58:19Z

See PR #20

zorglube · 2022-12-04T21:51:38Z

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 · 2022-12-04T21:59:09Z

@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 · 2022-12-04T22:07:04Z

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 · 2022-12-06T17:10:13Z

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.

Full refactoring :

bc9b50a

- Handle AutoCloseable Connexion and Database - Improved Resource loading

This was referenced Nov 29, 2022

Improved Resource loading #8

Closed

Handle AutoCloseable connexion and database #9

Closed

Maven small refactoring

178d3f4

Formating

19db1e8

lbruun reviewed Dec 4, 2022

View reviewed changes

lbruun closed this Dec 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring #19

Refactoring #19

zorglube commented Nov 29, 2022

zorglube commented Nov 29, 2022

lbruun commented Nov 29, 2022

zorglube commented Nov 30, 2022

zorglube commented Dec 2, 2022

lbruun Dec 4, 2022

zorglube Dec 4, 2022

lbruun Dec 4, 2022

zorglube Dec 4, 2022

lbruun Dec 4, 2022

zorglube Dec 4, 2022

lbruun Dec 4, 2022

lbruun Dec 4, 2022

lbruun commented Dec 4, 2022

lbruun commented Dec 4, 2022

zorglube commented Dec 4, 2022 •

edited

zorglube commented Dec 4, 2022

zorglube commented Dec 4, 2022

lbruun commented Dec 6, 2022

Refactoring #19

Refactoring #19

Conversation

zorglube commented Nov 29, 2022

zorglube commented Nov 29, 2022

lbruun commented Nov 29, 2022

zorglube commented Nov 30, 2022

zorglube commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbruun commented Dec 4, 2022

lbruun commented Dec 4, 2022

zorglube commented Dec 4, 2022 • edited

zorglube commented Dec 4, 2022

zorglube commented Dec 4, 2022

lbruun commented Dec 6, 2022

zorglube commented Dec 4, 2022 •

edited