feat: improve artisan migrate:status command #780
Conversation
WalkthroughThis pull request introduces several enhancements across multiple files, focusing on improving migration status handling and console output formatting. The primary changes include adding a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Context
participant Migrator as Database Migrator
participant Command as Migration Status Command
CLI->>Migrator: Request Migration Status
Migrator-->>Command: Return Migration Statuses
Command->>CLI: Use TwoColumnDetail to Display
CLI->>CLI: Format and Print Statuses
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #780 +/- ##
==========================================
- Coverage 69.75% 69.73% -0.02%
==========================================
Files 214 214
Lines 18337 18357 +20
==========================================
+ Hits 12791 12802 +11
- Misses 4843 4853 +10
+ Partials 703 702 -1 ☔ View full report in Codecov by Sentry. |
|
Notice: please post a comment when the PR is ready. |
|
Review Ready |
almas1992
force-pushed
the
optimize-migration-status-command
branch
2 times, most recently
from
a18d8b9 to
69c65d1
Compare
almas1992
force-pushed
the
optimize-migration-status-command
branch
from
69c65d1 to
7189f45
Compare
![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 comments (1)
database/migration/sql_migrator.go (1)
Line range hint
117-134: Consider enhancing error handling and return valuesThe Status() method could be improved in several ways:
- The method returns
nil, nilin multiple scenarios which makes it harder for callers to distinguish between different cases- The success message about migration version is printed inside this method, which mixes concerns
Consider this refactoring:
func (r *SqlMigrator) Status() ([]migration.Status, error) { version, dirty, err := r.migrator.Version() if err != nil { if errors.Is(err, migrate.ErrNilVersion) { - color.Warningln("No migrations found") - return nil, nil + return []migration.Status{}, nil } else { return nil, errors.MigrationGetStatusFailed.Args(err) } } if dirty { - color.Warningln("Migration status: dirty") + return []migration.Status{}, errors.New("Migration status: dirty") } - color.Successln(fmt.Sprintf("Migration version: %d", version)) - return nil, nil + return []migration.Status{{ + Version: version, + Dirty: dirty, + }}, nil🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-125: database/migration/sql_migrator.go#L125
Added line #L125 was not covered by tests
🧹 Nitpick comments (8)
database/migration/default_migrator.go (2)
115-134: Refactor for improved separation of concerns
In the current implementation of Status(), the function logs warnings and informational messages (e.g., “Migration table not found” or “No migrations found”) and also returns data. Moving console output to the caller or a dedicated utility would keep this core logic more concise and testable.
159-160: Preallocate the slice to reduce allocations
Consider declaring the slice with an initial capacity to optimize memory usage, for example:
var migrationStatus = make([]contractsmigration.Status, 0, len(batches))database/console/migration/migrate_status_command.go (1)
43-56: Comprehensive status output with color
The approach to print both columns using color-coded statuses is neat. However, the entire block is uncovered by tests, per the static analysis. You can add a test that invokes this command and verifies the output (possibly using a mocked console).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 43-53: database/console/migration/migrate_status_command.go#L43-L53
Added lines #L43 - L53 were not covered by tests
[warning] 55-55: database/console/migration/migrate_status_command.go#L55
Added line #L55 was not covered by testsdatabase/migration/sql_migrator_test.go (2)
136-138: Consider using s.Empty() instead of s.Nil() for slices
When checking that status is empty, it’s more semantically clear to use the assertion for emptiness:- s.Nil(status) + s.Empty(status)This more precisely checks that the slice has zero length (rather than being nil).
145-147: Use s.Empty() for slice checks
For consistency and clarity, do the same change as above here:- s.Nil(status) + s.Empty(status)console/cli_context_test.go (1)
269-289: Consider adding more test casesWhile the current test cases are good, consider adding these scenarios:
- Empty strings for both columns
- Very long strings that exceed terminal width
- Custom filler character using the optional rune parameter
Here's an example of additional test cases:
tests := []struct { name string first string second string + filler rune output string }{ // ... existing test cases ... + { + name: "empty strings", + first: "", + second: "", + output: color.Default().Sprintln(" " + color.Gray().Sprint( + strings.Repeat(".", pterm.GetTerminalWidth()-6), + ) + " "), + }, + { + name: "custom filler", + first: "Test", + second: "Result", + filler: '-', + output: color.Default().Sprintln(" Test " + color.Gray().Sprint( + strings.Repeat("-", pterm.GetTerminalWidth()-len("Test")-len("Result")-6), + ) + " Result "), + }, }console/cli_context.go (1)
282-308: LGTM! The implementation handles terminal width and ANSI colors correctly.The TwoColumnDetail method effectively formats two-column output with proper handling of:
- Unicode character widths using runewidth
- ANSI color codes
- Terminal width constraints
- Dynamic filling
Consider these minor improvements:
- Pre-allocate the strings.Builder capacity:
func (r *CliContext) TwoColumnDetail(first, second string, filler ...rune) { margin := func(s string, left, right int) string { - var builder strings.Builder + builder := strings.Builder{} + builder.Grow(len(s) + left + right) if left > 0 { builder.WriteString(strings.Repeat(" ", left)) }
- Extract the default filler character to a constant:
+const defaultFiller = '.' + func (r *CliContext) TwoColumnDetail(first, second string, filler ...rune) { // ... - fillingText = color.Gray().Sprint(strings.Repeat(string(append(filler, '.')[0]), w)) + fillingText = color.Gray().Sprint(strings.Repeat(string(append(filler, defaultFiller)[0]), w))mocks/console/Context.go (1)
1229-1241: Consider adding error handling for type assertionsWhile the implementation is functionally correct, consider adding error handling for the type assertions to make the mock more robust.
func (_c *Context_TwoColumnDetail_Call) Run(run func(first string, second string, filler ...rune)) *Context_TwoColumnDetail_Call { _c.Call.Run(func(args mock.Arguments) { variadicArgs := make([]rune, len(args)-2) for i, a := range args[2:] { if a != nil { - variadicArgs[i] = a.(rune) + if r, ok := a.(rune); ok { + variadicArgs[i] = r + } else { + panic(fmt.Sprintf("argument %d is not of type rune", i+2)) + } } } run(args[0].(string), args[1].(string), variadicArgs...) }) return _c }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
console/cli_context.go(2 hunks)console/cli_context_test.go(2 hunks)contracts/console/command.go(1 hunks)contracts/database/migration/migrator.go(2 hunks)database/console/migration/migrate_status_command.go(2 hunks)database/migration/default_migrator.go(3 hunks)database/migration/default_migrator_test.go(8 hunks)database/migration/sql_migrator.go(3 hunks)database/migration/sql_migrator_test.go(1 hunks)mocks/console/Context.go(1 hunks)mocks/database/migration/Migrator.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
database/console/migration/migrate_status_command.go
[warning] 39-40: database/console/migration/migrate_status_command.go#L39-L40
Added lines #L39 - L40 were not covered by tests
[warning] 43-53: database/console/migration/migrate_status_command.go#L43-L53
Added lines #L43 - L53 were not covered by tests
[warning] 55-55: database/console/migration/migrate_status_command.go#L55
Added line #L55 was not covered by tests
database/migration/sql_migrator.go
[warning] 125-125: database/migration/sql_migrator.go#L125
Added line #L125 was not covered by tests
🔇 Additional comments (17)
database/migration/default_migrator.go (1)
172-180: Implementation is correct
Appended fields align well with the new contractsmigration.Status struct. No functional issues found.
contracts/database/migration/migrator.go (2)
8-12: Excellent approach to typed migration status
Defining the Status struct clarifies the data model and improves maintainability.
26-26: Signature change fosters clearer status reporting
Switching from returning error-only to returning ([]Status, error) offers richer migration info.
database/console/migration/migrate_status_command.go (1)
39-40: Add test coverage for lines 39-40
The static analysis flags these lines for missing coverage. Consider adding unit tests or integration tests to ensure the status retrieval and error handling logic is validated.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-40: database/console/migration/migrate_status_command.go#L39-L40
Added lines #L39 - L40 were not covered by tests
database/migration/sql_migrator.go (1)
125-125:
Add test coverage for error handling
The error handling branch at line 125 is not covered by tests according to static analysis.
Please add test cases to cover the error handling scenario. I can help generate the test cases if needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 125-125: database/migration/sql_migrator.go#L125
Added line #L125 was not covered by tests
contracts/console/command.go (1)
73-74: LGTM! Clean interface addition
The new TwoColumnDetail method is well-defined with flexible parameters that allow for customized formatting.
console/cli_context_test.go (1)
292-294: LGTM! Clean test implementation
The test implementation is clean and uses appropriate test utilities for output capture and validation.
mocks/database/migration/Migrator.go (2)
252-278: LGTM! The mock implementation correctly handles the new Status return type.
The changes properly implement the updated Status method signature with appropriate type assertions and error handling.
298-299: LGTM! The mock helper methods are correctly updated.
The Return and RunAndReturn methods are properly updated to handle the new Status return type.
Also applies to: 303-304
database/migration/default_migrator_test.go (3)
91-93: LGTM! TestRun properly verifies the migration status.
The test correctly validates both the migration execution and its status.
142-144: LGTM! TestStatus comprehensively tests the status functionality.
The test covers both initial empty state and post-migration state with proper assertions.
Also applies to: 152-160
788-790: LGTM! The Status test cases cover all scenarios.
The test suite thoroughly covers:
- Migration table not found
- Error handling
- No migrations
- Successful status retrieval with proper batch information
Also applies to: 801-803, 815-817, 837-849
mocks/console/Context.go (5)
1203-1214: LGTM: TwoColumnDetail method implementation is correct
The implementation properly handles the variadic arguments and follows the established pattern in the mock file.
1215-1219: LGTM: Context_TwoColumnDetail_Call struct is properly defined
The struct definition correctly embeds mock.Call and follows the established pattern.
1220-1228: LGTM: TwoColumnDetail expecter method is well implemented
The method properly handles all parameters, includes appropriate documentation, and follows the established pattern.
1242-1250: LGTM: Return and RunAndReturn methods are properly implemented
Both methods follow the established pattern and are correctly implemented.
1203-1250: Verify interface compliance
Let's verify that the mock implementation matches the interface definition.
✅ Verification successful
Mock implementation matches the interface definition
The mock implementation in mocks/console/Context.go correctly matches the interface definition found in contracts/console/command.go. The method signature TwoColumnDetail(first, second string, filler ...rune) is identical in both the interface and the mock implementation, including the parameter types and variadic rune parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the TwoColumnDetail method signature matches the interface definition
# Search for the interface definition
ast-grep --pattern 'interface {
$$$
TwoColumnDetail($$$)
$$$
}'
Length of output: 4696
📑 Description
CliContextaddTwoColumnDetailmethod.Screenshots
Summary by CodeRabbit
New Features
TwoColumnDetailmethod for improved console output formatting.Bug Fixes
Tests
TwoColumnDetailmethod.Chores
✅ Checks