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

Upsert + Refactor BaseBuilder *Batch() Methods #6407

Closed

Conversation

sclubricants
Copy link
Member

@sclubricants sclubricants commented Aug 22, 2022

This PR is a combination of #6367 and #6373 as well as refactoring all the Batch methods.

This is the bigger picture of where those PR's are headed. You can consider this PR or not. Its hard to break things down much smaller because everything is interdependent.

I would suggest creating a special branch for upsert and refactor of Batch methods. Perhaps we could merge this PR there instead. This way we all may work together on it and contribute to it. I welcome any help!

This fixes #5674

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added 4.3 enhancement PRs that improve existing functionalities labels Aug 22, 2022
@sclubricants sclubricants added refactor Pull requests that refactor code database Issues or pull requests that affect the database layer labels Aug 22, 2022
@sclubricants sclubricants force-pushed the RefactorBaseBuilderBatch branch 3 times, most recently from 7ddeb54 to cc0f38f Compare August 23, 2022 00:04
@sclubricants
Copy link
Member Author

sclubricants commented Aug 24, 2022

We must be running an older version of SQLite somewhere. Coveralls:

Capture

@sclubricants sclubricants force-pushed the RefactorBaseBuilderBatch branch from af34872 to cc25ef4 Compare August 24, 2022 17:37
@sclubricants sclubricants force-pushed the RefactorBaseBuilderBatch branch from cc25ef4 to 1423fda Compare August 25, 2022 18:30
@kenjis kenjis marked this pull request as draft August 26, 2022 21:46
@kenjis
Copy link
Member

kenjis commented Aug 26, 2022

This PR is too big (+2,441, −267), and the commit is also too big.
It is not acceptable, because it has multiple changes.

One thing at a time: A pull request should only contain one change.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#branching

The change in one commit is also so large that it is difficult to understand the code.
I believe that merging this would be an obstacle to future maintenance.
I hope a PR like this size has more than 200 commits.

So I think this PR is just a reference.

@kenjis
Copy link
Member

kenjis commented Aug 26, 2022

Our PR guideline does not mention about commit messages well now.
I put some references for writing good commit messages:

@sclubricants
Copy link
Member Author

sclubricants commented Aug 26, 2022

I would suggest creating a special branch for upsert and refactor of Batch methods. Perhaps we could merge this PR there instead.

@kenjis
Copy link
Member

kenjis commented Aug 26, 2022

If we create special branch, can you send small PRs to it?

@sclubricants
Copy link
Member Author

I can try. A lot of the changes take place simultaneously.

The only way I could properly plan and put it all together was to put it all together at once. There are things shared between upsert and update and there are things shared between upsert and insert. I had to put them all together to normalize these common things. Then refactor the methods that set and execute them.

It would be a lot of piece work to try and do it one by one. I could summarize the major changes though.

I had all the commits at one time but you complained there were too many so I merged them all together.

@sclubricants
Copy link
Member Author

There are a few things Id like to discuss with what is here. Once those things are figured out then I can rebuild it piece by piece if necessary.

@sclubricants
Copy link
Member Author

The old code was really screwy. Things were being set in all different places. Now there is one method to set data.. setBatch(). Any data that comes in through the other methods is referred to setBatch()

@sclubricants
Copy link
Member Author

Create a new branch for this and I will submit it in pieces and explain each piece.

@kenjis
Copy link
Member

kenjis commented Aug 27, 2022

I had all the commits at one time but you complained there were too many so I merged them all together.

I don't recall complaining about too many PR commits, but maybe there was a problem with my English.

I recommend atomic commits. Also, I would like the overall size of a PR to be small.

@sclubricants
Copy link
Member Author

Its best to review this code by looking at the code and not the "files changed".

@sclubricants sclubricants added can of worms and removed enhancement PRs that improve existing functionalities database Issues or pull requests that affect the database layer refactor Pull requests that refactor code 4.3 labels Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants