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.

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

Insert or replace async #26

Merged
merged 1 commit into from May 12, 2014

Conversation

rudmer90
Copy link

@rudmer90 rudmer90 commented May 8, 2014

Noticed that the async library was missing InsertOrReplaceAsync() method, which I've found useful in the synchronous version, so I added it along with a InsertOrReplaceAll() and corresponding InsertOrReplaceAllAsync() methods and tests. @softlion has something similar in his large pull request, but that's been hanging around for a few months and I don't know if he has any plans to further pursue merging it.

Sorry for the ugly diff on SQLIteConnection.cs -- the intermingling of the Insert(), InsertAll(), and InsertOrReplace() methods really bugged me (makes them harder to find), so I segregated them. The only new thing in that file is the InsertOrReplaceAll() method.

@softlion
Copy link

softlion commented May 8, 2014

I"m still following the updates, if any, after verifying all code. But i'm pretty happy with the performance stability and concurrency of my version, after specifying some new options i've not pulled yet. Of course it is still not a real IQueryable, which is likely what any of us would like to have.

@oysteinkrog
Copy link
Owner

This looks good except for one thing; reordering the methods is not OK.
I try to keep all files formatted with the included ReSharper settings (the method ordering is set to the defaults).
I can understand why you want a different order but it's a bad idea for several reasons, primarily because everyone has a different opinion on what the best order is.
I fear your change here would be reverted as soon as someone reformats the file.

@softlion
Copy link

What do you mean by reordering ? Changing the line at which a method starts ?

@rudmer90
Copy link
Author

Alright, I'll change the order back and resubmit. :)

@oysteinkrog
Copy link
Owner

Thanks

@rudmer90
Copy link
Author

So I reverted SQLiteConnection.cs to the version prior to all of my changes, and then simply added the two methods I wrote back in. The diff still looks ugly for some reason, but if you compare the end result of the file against what it was before my changes in a side-by-side differ you should see that everything is back in order. EDIT: I was looking at the wrong diff. The final diff looks great, all set. :)

@oysteinkrog
Copy link
Owner

I'm sorry, but you need to squash these, the problem now is that this history is not very nice to merge into master, and anyone that has forks/branches based on this with changes in the places you've touched will have conflicts when rebasing.
After you cleanup your local commits, just force push and the PR will automatically be updated.

@rudmer90
Copy link
Author

Well that was painful, but they're all squashed now.

@oysteinkrog
Copy link
Owner

Thank you, I am almost happy now.
Please also fix the commit message (here is a quick link about this https://github.com/erlang/otp/wiki/Writing-good-commit-messages). I don't know what kind of client you are using but you should be able to do it using an amend commit (commit again with the amend option and change the message).

Please forgive my pedantic behavior here, I just think these kinds of things are very important for open projects as it can quickly become a huge mess otherwise.
Pain builds character:)

… following functions (and their corresponding unit tests):
- SQLiteConnection.InsertOrReplaceAll()
- SQLiteAsyncConnection.InsertOrReplaceAsync()
- SQLiteAsyncConnection.InsertOrReplaceAllAsync() This single commit was previously several commits that were pushed to a branch on my fork,
but have since been squashed for history/rebase purposes. The squashed commits were: [67db56c] Reverting method reordering, readding SQLiteConnection.InsertOrReplaceAll() methods.
[0499a32] Adding unit tests for new InsertOrReplace() and InsertOrReplaceAll() methods.
[d5dd60b] Adding SQLiteConnection.InsertOrReplaceAll() and SQLiteAsyncConnection.InsertOrReplaceAllAsync() methods.
[47de3e1] Adding SQLiteAsyncConnection.InsertOrReplaceAsync() method.
@rudmer90
Copy link
Author

No problem. This is my first real contribution to an open source project, so I appreciate your patience. Next time will be much smoother, hopefully. :)

I followed the style guide you linked with my commit message, including a line break after the summary, but Github is wanting to take the next line and shove it up into the first. Is this acceptable? I have no idea how to make it not do that.

@oysteinkrog
Copy link
Owner

Yeah that's OK:)
I'm not sure why you have that problem but it's not that important.
Edit: Hmm, nvm, this is OK.
Thanks for the contribution and congratulations on your first contribution to open source!
I hope it was not too painful:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants