-
Notifications
You must be signed in to change notification settings - Fork 25
The "description" method has been corrected #104
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
base: master
Are you sure you want to change the base?
Conversation
sorinsarca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @SMI-82, thank you for your contribution!
I would like to bring to your attention a few things:
- one pull-request per feature, I usually don't merge multiple unreleated changes
- make sure you don't break backward compatibility
- add tests for your changes
- it is always better to discuss if something should be added (open an issue)
This project supports multiple database systems (MySQL, PostgreSQL, Microsoft SQL, SQLite, etc.) but most of your features are MySQL only.
You can find more details in the review attached to this PR.
| /** | ||
| * @return $this | ||
| */ | ||
| public function nullable(): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Columns are nullable by default, this is redundant.
| * @param string $value | ||
| * @return $this | ||
| */ | ||
| public function collate(string $value): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different DBs use different values for COLLATE.
| /** | ||
| * @return $this | ||
| */ | ||
| public function onUpdateCurrentTimeStamp(): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MySQL specific.
You can implement this in most DBs using a TRIGGER.
| * @return string | ||
| */ | ||
| protected function handleModifierComment(BaseColumn $column): string | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MySQL only.
| * @return $this | ||
| */ | ||
| public function timestamps(string $createColumn = 'created_at', string $updateColumn = 'updated_at'): self | ||
| public function timestamps($fields = []): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaks backward compatibility!
| * @param BaseColumn $column | ||
| * @return string | ||
| */ | ||
| protected function handleModifierOnUpdateCurrentTimeStamp(BaseColumn $column): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MySQL only.
Hello! 👋
I’d like to contribute to your project. This pull request includes a few fixes that improve the functionality. Looking forward to your feedback!