feat: consolidate schema options formatting #241
Conversation
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/Concerns/SharedGrammarCalls.php (2)
48-59: Add documentation examples and handle empty array case.The method looks good but could benefit from these improvements:
- Add usage examples in the docblock
- Consider handling empty array case explicitly
/** * @param array<string, scalar|BackedEnum> $options * @param string $delimiter * @return string + * @example + * formatOptions(['max_length' => 100, 'nullable' => true]) // 'max_length=100, nullable=true' + * formatOptions(['enum_value' => Status::ACTIVE]) // 'enum_value=ACTIVE' */ protected function formatOptions(array $options, string $delimiter = '='): string { + if (empty($options)) { + return ''; + } $mapped = Arr::map($options, function (int|float|bool|string|BackedEnum $v, string $k) use ($delimiter): string { return Str::snake($k) . $delimiter . $this->formatOptionValue($v); }); return implode(', ', $mapped); }
61-73: Enhance type safety and validation.While the implementation is clean, consider these improvements for better type safety:
- The
mixedtype hint with@param scalar|BackedEnumcould be misleading- Add validation for unexpected types
/** * @param scalar|BackedEnum $value * @return string + * @throws \InvalidArgumentException If value is not scalar or BackedEnum */ - protected function formatOptionValue(mixed $value): string + protected function formatOptionValue(int|float|string|bool|BackedEnum $value): string { return match (true) { is_bool($value) => $value ? 'true' : 'false', is_string($value) => $this->quoteString($value), $value instanceof BackedEnum => $this->formatOptionValue($value->value), - default => (string) $value, + is_scalar($value) => (string) $value, + default => throw new \InvalidArgumentException('Unexpected value type'), }; }CHANGELOG.md (1)
Line range hint
1-10: Consider categorizing changes for better readability.While the changes are well-documented, consider organizing them into categories like "Added", "Changed", and "Deprecated" for better clarity. This would align with the format used in previous versions and make it easier for users to understand the impact of changes.
Example structure:
# v8.3.0 (2024-11-08) +Added: - add support for invisible columns (#240) - add support for change streams using Blueprint (#230) - add support for snapshot queries (#215) - add support for `Query\Builder::whereNotInUnnest(...)` (#225) +Changed: - consolidate schema options formatting (#241) - `Query\Builder::whereIn` will now wrap values in `UNNEST` if the number of values exceeds the limit (950). (#226) - Commit options can now be set through config or through `Connection::setCommitOptions(...)` (#229) +Deprecated: - deprecate Connection::getDatabaseContext() and move logic to UseMutations::getMutationExecutor() (#227)src/Schema/Grammar.php (1)
330-332: Consider simplifying the empty check.The current implementation could be more concise.
- return $options !== [] - ? 'options (' . $this->formatOptions($options) . ')' - : ''; + return $options === [] ? '' : 'options (' . $this->formatOptions($options) . ')';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/Concerns/SharedGrammarCalls.php(2 hunks)src/Schema/Grammar.php(2 hunks)
🔇 Additional comments (5)
src/Concerns/SharedGrammarCalls.php (2)
20-23: LGTM! Required imports for the new functionality.
48-73: Verify the usage of new formatting methods.
Let's confirm that these new methods are being used consistently across the codebase.
✅ Verification successful
Let me try a different approach to verify the usage of these methods.
Methods are used correctly in the Schema Grammar implementation
The verification shows that these formatting methods are properly used within the codebase:
formatOptions()is called insrc/Schema/Grammar.phpfor handling schema optionsformatOptionValue()is used internally withinSharedGrammarCallstrait for value formatting- The implementation follows a consistent pattern for options formatting across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of formatOptions and formatOptionValue methods
# Expected: Find calls in formatSequenceOptions and formatChangeStreamOptions
echo "Checking for method usage..."
ast-grep --pattern 'function formatSequenceOptions($$$) {
$$$
formatOptions($$$)
$$$
}'
ast-grep --pattern 'function formatChangeStreamOptions($$$) {
$$$
formatOptions($$$)
$$$
}'
Length of output: 262
Script:
#!/bin/bash
# Search for any calls to formatOptions and formatOptionValue
echo "Searching for formatOptions calls..."
rg "formatOptions\(" -A 2
echo -e "\nSearching for formatOptionValue calls..."
rg "formatOptionValue\(" -A 2
echo -e "\nSearching for class implementations..."
ast-grep --pattern 'trait SharedGrammarCalls {
$$$
}'
Length of output: 1731
CHANGELOG.md (1)
Line range hint 12-1000: LGTM! The changelog maintains good documentation practices.
The changelog demonstrates consistent formatting, clear marking of breaking changes, and proper referencing of PRs. The inclusion of important notes about version requirements and the clear categorization of changes in previous versions makes it easy for users to understand the evolution of the codebase.
src/Schema/Grammar.php (2)
265-265: LGTM! Method signature improvements enhance type safety.
The changes to method signatures improve type safety by:
- Adding explicit return types
- Making the parameter type more flexible by using
objectinstead ofFluent
Also applies to: 330-332
265-265: LGTM! Improved code maintainability through consolidation.
The refactoring to use formatOptions from the SharedGrammarCalls trait reduces code duplication and improves consistency in option formatting.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
CHANGELOG.mdto reflect the latest version and changes.