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

INSERT tests and fixes #199

Closed
wants to merge 13 commits into from
Closed

INSERT tests and fixes #199

wants to merge 13 commits into from

Conversation

KyGost
Copy link
Contributor

@KyGost KyGost commented Apr 2, 2021 •
edited
Loading

Resolve: #197
Resolve: #190
Resolve: #189

@KyGost KyGost marked this pull request as draft April 2, 2021 03:23
@KyGost KyGost mentioned this pull request Apr 2, 2021
Closed
@panarch
Copy link
Member

panarch commented Apr 2, 2021 •
edited
Loading

I really appreciate on this work, thanks! solving three insertion issues.
However, if you are ok then could you request review by making an independent pr per each issue?
For me, reviewing multiple contexts at the same time really requires much more effort to do look around.

@KyGost
Copy link
Contributor Author

KyGost commented Apr 2, 2021

Please see it as improving insertion code and adding better tests and just also solving some bugs at the same time as that is what this is.

@KyGost
Copy link
Contributor Author

KyGost commented Apr 2, 2021

Oh oops I didn't push my recent changes

@KyGost KyGost marked this pull request as ready for review April 2, 2021 10:13
@KyGost
Auto increment columns (#3)
9a1bd93 
* Remade for v0.5

* Clippy

* Appease no defaults clippy

* Clean up generate

* Remove sled transaction and use ColumnOptionExt for AutoIncrement column option check

* Clip

* Improvements

* Clippy

* No Mut change

* Minimize work in storage

* Split get and set

* Clip

* Move row mutation to end, make procedures occur in two segments.

* Clean up a little, Clip

* Better testing

* Stuff for clippy reasons

* Revert "Stuff for clippy reasons"

This reverts commit 99956a6.

* Ignore clippy stuff for now, should be fixed in different PR

* Use a transaction

* Clippy and remove acceptance of AUTOINCREMENT
@KyGost
Api select json (#1)
8de7b04 
* Working

* Move more into optional feature
@KyGost
Merge branch 'main' into INSERT_fixes
bd401a7
@KyGost
Copy link
Contributor Author

KyGost commented Apr 3, 2021

Why are these in this PR?!?!?!
Also I forgot to remove a test println, fixing now.

@panarch
Copy link
Member

panarch commented Apr 4, 2021

Oh.. now I see that..
Ok, this is really too much for me to do review at once.

These are independent issues, if you can split this PR into three, then I can fastly review one by one.
However, I'm not sure I can do review for this one.

@KyGost
Copy link
Contributor Author

KyGost commented Apr 4, 2021

Please, once again, ignore the fixes.

This is a rewrite of how we compile rows and insert data.

@KyGost KyGost mentioned this pull request Apr 4, 2021
Merged
@KyGost
AUTO_INCREMENT (#1)
9c0fb6d 
* Remade for v0.5

* Clippy

* Appease no defaults clippy

* Clean up generate

* Remove sled transaction and use ColumnOptionExt for AutoIncrement column option check

* Clip

* Improvements

* Clippy

* No Mut change

* Minimize work in storage

* Split get and set

* Clip

* Move row mutation to end, make procedures occur in two segments.

* Clean up a little, Clip

* Better testing

* Stuff for clippy reasons

* Revert "Stuff for clippy reasons"

This reverts commit 99956a6.

* Ignore clippy stuff for now, should be fixed in different PR

* Use a transaction

* Clippy and remove acceptance of AUTOINCREMENT
@KyGost
Merge branch 'main' into INSERT_fixes
749ee06
@panarch
Copy link
Member

panarch commented Apr 24, 2021

Closing PRs which are merged and working well in the fork.

@panarch panarch closed this Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@KyGost @panarch