Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 40 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesInitial copy of Derby extension #4424
Conversation
This comment has been minimized.
This comment has been minimized.
Looks like format validation failed. Can you run 'mvn process-sources' and push an amended commit? We have really strict formatting rules but the default maven build automatically formats, so if you build from maven before pushing you should not run into this again. |
...src/main/java/io/quarkus/jdbc/derby/deployment/DerbyJDBCReflections.java
Outdated
Show resolved
Hide resolved
integration-tests/jpa-derby/src/main/resources/META-INF/beans.xml
Outdated
Show resolved
Hide resolved
...t/src/main/java/io/quarkus/jdbc/derby/deployment/JDBCDerbyProcessor.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Thanks for the review comments already @stuartwdouglas and @cescoffier! This PR was just a draft so not fully polished yet, but I appreciate you guys taking the time to look it over in the early stages. |
This comment has been minimized.
This comment has been minimized.
@stuartwdouglas I have been running various flavors of I'm not too familiar with azure pipelines, but I take it the formatting failure is described somewhere in the build output? I've tried to view the build results myself (specifically this link) but it's not loading for me -- perhaps they have been auto-deleted by now? |
Overall, looks good to me (hi Andy!). I added a couple of comments here and there. |
...ment/src/main/java/io/quarkus/deployment/builditem/FeatureBuildItem.java
Outdated
Show resolved
Hide resolved
...src/main/java/io/quarkus/jdbc/derby/deployment/DerbyJDBCReflections.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/io/quarkus/jdbc/derby/deployment/JDBCDerbyProcessor.java
Show resolved
Hide resolved
...arkus/jdbc/derby/runtime/graal/ClientDriver_PlaceholderSubstitution.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
That link worked fine for me, AFAIK the results should be public so you should be able to see them as well. To see the logs you need to click on the 'Linux Maven Repo' task. If you run mvn install it should auto format for you. I also use the following in '.git/hooks/pre-push'
This will check before pushing (if you want to save time as you know it is ok you can use --no-verify when you push) |
This comment has been minimized.
This comment has been minimized.
PR is now updated with all review comment feedback, as well as CI checks passing. @Sanne @cescoffier @gsmet please re-review when you get a chance. Thanks! |
Looks good to me, but I'm not a database guy. |
public static final String JDBC_POSTGRESQL = "jdbc-postgresql"; | ||
public static final String JDBC_MARIADB = "jdbc-mariadb"; |
This comment has been minimized.
This comment has been minimized.
machi1990
Oct 14, 2019
Member
a very minor comment - I think the constants are sorted in alphabetically. We should preserve that :-)
Seems ready to go! Just missed to mention the new Maven modules in the respective parents, I added that. Thanks! |
aguibert commentedOct 7, 2019
•
edited
Working directly with @Sanne on this, but anyone is welcome to commend and discuss!
Fixes #4442