Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added initial support for Composite Foreign Keys and Indexes #19

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Added initial support for Composite Foreign Keys and Indexes #19

wants to merge 9 commits into from

Conversation

Trellian
Copy link
Contributor

Hi Sam,

My first PR of reasonable size. I'm new to this so please bear with me. Any and all feedback and criticism is very welcome.

Thanks for this awesome project, btw. Your mixing of FreeMarker with the APT is a stroke of genius IMO.

Regards,
Adrian

samkirton
samkirton reviewed Sep 22, 2016
View reviewed changes
.../com/memtrip/sqlking/preprocessor/processor/freemarker/method/AssembleCreateTableMethod.java
List<ForeignKey> foreignKeyList = table.getForeignKeys();
for (ForeignKey foreignKey : foreignKeyList)
{
StringBuilder sb_FK = new StringBuilder();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you split out the building of sb_FK into a separate method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, no problem

/Adrian

@samkirton
Copy link
Member

The PR looks good mate, could you also add a README on how to use the composites though?

Also see: https://github.com/memtrip/SQLKing/blob/master/client/src/tests/java/com/memtrip/sqlking/integration/IndexTest.java

For examples of how I was testing whether indexes were created successfully.

Cheers,
Sam

@Trellian
Added initial support for Composite Foreign Keys and Indexes, Table C…
0bfb271 
…onstraints and Triggers
@Trellian
Merge branch 'feature/composite_indexes_fks' of https://github.com/Tr…
802006c 
…ellian/SQLKing.git into feature/composite_indexes_fks

# Conflicts:
#	client/build.gradle
#	common/pom.xml
#	common/src/main/java/com/memtrip/sqlking/common/Column.java
#	common/src/main/java/com/memtrip/sqlking/common/ForeignKey.java
#	common/src/main/java/com/memtrip/sqlking/common/Index.java
#	common/src/main/java/com/memtrip/sqlking/common/IndexColumn.java
#	common/src/main/java/com/memtrip/sqlking/common/RIRule.java
#	common/src/main/java/com/memtrip/sqlking/common/SortOrder.java
#	common/src/main/java/com/memtrip/sqlking/common/Table.java
#	preprocessor/pom.xml
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/Processor.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/data/Column.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/data/Data.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/data/ForeignKey.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/data/Index.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/data/IndexColumn.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/data/Table.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/data/parse/ParseColumnAnnotation.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/data/parse/ParseTableAnnotation.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/freemarker/DataModel.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/freemarker/method/AssembleCreateIndexesMethod.java
#	preprocessor/src/main/java/com/memtrip/sqlking/preprocessor/processor/freemarker/method/AssembleCreateTableMethod.java
#	preprocessor/src/main/resources/Q.java
#	preprocessor/src/test/java/com/memtrip/sqlking/preprocessor/User.java
@Trellian
Copy link
Contributor Author

Hi Sam,

I've added a few new features, including Table and Column constraints and Triggers. Also added some basic testing for Foreign Keys. Please take a peek at it and give me your feedback, I'm hoping that I'm going in the direction you were planning on.

Thanks,
Adrian

samkirton
samkirton reviewed Sep 26, 2016
View reviewed changes
client/src/tests/java/com/memtrip/sqlking/integration/CreateTest.java
@@ -101,7 +101,7 @@ public void testMultipleInsert() {
};

// exercise
Insert.getBuilder().values(users).execute(getSQLProvider());
Insert.getBuilder().values((Object[])users).execute(getSQLProvider());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the cast to Object[] required?

@samkirton
Copy link
Member

Its looking good mate, I will make a code formatter for us to use though so its consistent. I added one other comment.

Cheers,
Sam

@Trellian
Copy link
Contributor Author

I'm not sure... the compiler complained about not having it, so I added it, and it worked.
I'm not sure what changed to cause this behaviour :(

On 26 Sep 2016 12:02, Samuel Kirton wrote:

@samkirton commented on this pull request.

In client/src/tests/java/com/memtrip/sqlking/integration/CreateTest.java
#19 (review):

@@ -101,7 +101,7 @@ public void testMultipleInsert() {
};

      // exercise

  •    Insert.getBuilder().values(users).execute(getSQLProvider());

  •    Insert.getBuilder().values((Object[])users).execute(getSQLProvider());

Why is the cast to Object[] required?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#19 (review), or mute the
thread
https://github.com/notifications/unsubscribe-auth/ABCOrCS56z4FGSM-z_NMwIUaIGUOGm3eks5qt5hEgaJpZM4KDnxU.

@Trellian
Copy link
Contributor Author

Thanks for the code formatter, I've been using the other method for years, and I keep
forgetting to update the formatting after i've finished coding. I find the indented
bracket style more readable, but will try to adhere more consistently to the standard.

/Adrian

On 26 Sep 2016 12:03, Samuel Kirton wrote:

Its looking good mate, I will make a code formatter for us to use though so its
consistent. I added one other comment.

Cheers,
Sam

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#19 (comment), or mute the thread
https://github.com/notifications/unsubscribe-auth/ABCOrILCarDZHH9gePODwBCdVUhWEwCTks5qt5hzgaJpZM4KDnxU.

@Trellian
Copy link
Contributor Author

Trellian commented Oct 5, 2016 •
edited
Loading

Hi Sam,

Added and cleaned up, sorry, I forgot to create a new branch before adding the Trigger and Constraint annotations. Please take a peek and let me know what you think.

I still need to create some examples for the Trigger annotations in the Preprocessor tests, and create the necessary client tests, but I'm running very short on time at the moment.

Thanks,
Adrian

@samkirton
Copy link
Member

Thanks for the contribution Adrian, its really nice, could you run the default Intellji formatter over the code? I will then squash the commit and rebase it with master.

@Trellian
Relaxed support for table alias names not yet fully implemented, Fore…
d918648 
…ign key syntax was not being generated properly
@samkirton
Copy link
Member

Hi Adrian,

Is this pull request ready to get merged? I will reformat your code before rebasing it into master, so let me know if you need to add any more changes. Thanks again for your contribution.

Cheers,
Sam

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samkirton samkirton samkirton left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Trellian @samkirton