memtrip / SQLKing

SQLKing is an Android SQLite ORM powered by an annotation preprocessor, tables are defined by Table annotations and CRUD classes expose an expressive api for executing SQLite queries. @memtrip
Other
21 stars 9 forks source link

Added initial support for Composite Foreign Keys and Indexes #19

Open Trellian opened 8 years ago

Trellian commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 https://github.com/memtrip/SQLKing/pull/19#pullrequestreview-1506346:

@@ -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 https://github.com/memtrip/SQLKing/pull/19#pullrequestreview-1506346, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCOrCS56z4FGSM-z_NMwIUaIGUOGm3eks5qt5hEgaJpZM4KDnxU.

Trellian commented 8 years ago

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 https://github.com/memtrip/SQLKing/pull/19#issuecomment-249529638, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCOrILCarDZHH9gePODwBCdVUhWEwCTks5qt5hzgaJpZM4KDnxU.

Trellian commented 7 years ago

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 commented 7 years ago

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.

samkirton commented 7 years ago

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