CodeIgniter4 icon indicating copy to clipboard operation
CodeIgniter4 copied to clipboard

Add processIndexes() and dropPrimaryKey() to Forge

Open sclubricants opened this issue 3 years ago • 4 comments

Ref #4814 #4653

This PR adds functionality to create indexes, primary keys, and foreign keys on a table already created. It does this by calling the new function processIndexes() instead of createTable().

$forge->addKey(['category', 'name'])
      ->addPrimaryKey('id')
      ->addForeignKey('userid', 'user', 'id')
      ->processIndexes('actions');

Also a new method dropPrimaryKey() allows to drop the primary key.

There were some differences between DBMS of the naming of indexes. These have been consolidated so each platform follows the same convention.

Checklist:

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

sclubricants avatar Aug 27 '22 00:08 sclubricants

The commits are too big. Could you make small, atomic commits? And please write good commit messages, not just Fix. See https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention

kenjis avatar Aug 27 '22 02:08 kenjis

This PR fixes https://github.com/codeigniter4/CodeIgniter4/issues/6453

@kenjis I annotated all the changes below. Easier to read from the Files Changed tab.

If this doesn't suffice I can rebuild it in another branch. I will try and get better at commits and messages.

sclubricants avatar Aug 29 '22 22:08 sclubricants

@sclubricants I appreciate your efforts.

I don't seem to have communicated well what the problem is.

Small PR

The most important thing is to make a PR small. Small I mean is the count of diff lines +825, −248 is small. In this case, abs(825) + abs(-248) = 1,073. I hope it is less than 500.

To make a PR small, we can split a big thing into slices. It seems this PR includes:

  1. processIndexes()
  2. dropPrimaryKey()
  3. the naming of indexes consolidation

Atomic Commit

Next, atomic commit. This will help in understanding big PRs. You put many comments on GitHub, but when you explain one thing, there should be one commit (or more). But the first commit has too many your comments. It shows the commit is too big.

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#committing

kenjis avatar Aug 30 '22 02:08 kenjis

  1. processIndexes()
  2. dropPrimaryKey()
  3. the naming of indexes consolidation

I work on this this weekend and break apart into seperate PRs.

sclubricants avatar Sep 01 '22 15:09 sclubricants