-
-
Notifications
You must be signed in to change notification settings - Fork 466
refactor: docker status parsing #886
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
Conversation
Reviewer's GuideThis PR refactors container state handling by introducing a ContainerStatus enum (with parsing, display, and utility methods) to replace the old running boolean, updates both DockerPs and PodmanPs to emit status, adjusts UI code to use the new status API, and expands tests to validate parsing and status logic. Class diagram for ContainerPs, DockerPs, PodmanPs, and ContainerStatus changesclassDiagram
class ContainerPs {
<<sealed>>
String? image
String? name
String? cmd
ContainerStatus status
String? cpu
String? mem
void parseStats(String s)
}
class DockerPs {
String? state
ContainerStatus status
void parseStats(String s)
ContainerStatus get status => ContainerStatus.fromDockerState(state)
}
ContainerPs <|-- DockerPs
class PodmanPs {
bool? exited
ContainerStatus status
void parseStats(String s)
ContainerStatus get status => ContainerStatus.fromPodmanExited(exited)
}
ContainerPs <|-- PodmanPs
class ContainerStatus {
<<enum>>
running
exited
created
paused
restarting
removing
dead
unknown
bool get isRunning
bool get canStart
bool get canStop
bool get canRestart
static ContainerStatus fromDockerState(String? state)
static ContainerStatus fromPodmanExited(bool? exited)
String get displayName
}
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@claude review |
|
Claude finished @lollipopkit's task —— View job Docker Status Parsing Refactor ReviewThis is a well-executed refactoring that addresses Docker container status parsing. Here's my comprehensive review: Todo List:
🎯 Overall Assessment: EXCELLENT ✅This refactoring significantly improves the container status handling system with better type safety, extensibility, and robustness. ✅ Strengths1. Strong Type Safety Improvement
2. Robust Docker State Parsing (
|
| Aspect | Rating | Notes |
|---|---|---|
| Type Safety | ⭐⭐⭐⭐⭐ | Excellent enum-based approach |
| Error Handling | ⭐⭐⭐⭐⭐ | Comprehensive null/edge case handling |
| Test Coverage | ⭐⭐⭐⭐⭐ | Thorough testing with edge cases |
| API Design | ⭐⭐⭐⭐⭐ | Intuitive helper methods |
| Performance | ⭐⭐⭐⭐⭐ | No performance concerns |
| Maintainability | ⭐⭐⭐⭐⭐ | Well-structured and extensible |
✅ Verification Points
- ✅ Docker parsing handles
Up X minutes/hours/dayscorrectly - ✅ Podman parsing properly converts
exitedboolean to status - ✅ UI integration maintains display functionality
- ✅ Menu system correctly uses
isRunningfor action availability - ✅ Edge cases like null states handled gracefully
🏆 Conclusion
This refactoring successfully addresses issue #795 by implementing a robust, type-safe container status parsing system. The comprehensive test coverage and thoughtful API design make this a high-quality improvement that enhances both reliability and maintainability.
Recommendation: APPROVE ✅
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider moving the hard-coded status.displayName strings into your l10n resources to support localization instead of using English literals.
- Unify the UI status display by using ContainerStatus.displayName for DockerPs items as well (instead of raw state) so both Docker and Podman show statuses consistently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the hard-coded status.displayName strings into your l10n resources to support localization instead of using English literals.
- Unify the UI status display by using ContainerStatus.displayName for DockerPs items as well (instead of raw state) so both Docker and Podman show statuses consistently.
## Individual Comments
### Comment 1
<location> `lib/data/model/container/status.dart:53` </location>
<code_context>
+ }
+
+ /// Get display string for the status
+ String get displayName {
+ return switch (this) {
+ ContainerStatus.running => 'Running',
</code_context>
<issue_to_address>
Hardcoded display names may limit localization support.
Consider using a localization mechanism for displayName to support multiple languages if your app requires internationalization.
Suggested implementation:
```
/// Get localized display string for the status
String displayName(BuildContext context) {
final localizations = AppLocalizations.of(context);
return switch (this) {
ContainerStatus.running => localizations.containerStatusRunning,
ContainerStatus.exited => localizations.containerStatusExited,
ContainerStatus.created => localizations.containerStatusCreated,
ContainerStatus.paused => localizations.containerStatusPaused,
ContainerStatus.restarting => localizations.containerStatusRestarting,
ContainerStatus.removing => localizations.containerStatusRemoving,
ContainerStatus.dead => localizations.containerStatusDead,
ContainerStatus.unknown => localizations.containerStatusUnknown,
};
}
```
1. You must add the corresponding keys (e.g., `containerStatusRunning`, `containerStatusExited`, etc.) to your `intl` ARB files and regenerate your localization classes.
2. Update all usages of `displayName` to pass a `BuildContext` parameter, e.g., `status.displayName(context)`.
3. Ensure `AppLocalizations` is imported: `import 'package:flutter_gen/gen_l10n/app_localizations.dart';`
</issue_to_address>
### Comment 2
<location> `lib/view/page/container/container.dart:223` </location>
<code_context>
- final PodmanPs ps => ps.running ? l10n.running : l10n.stopped,
- final DockerPs ps => ps.state,
+ final PodmanPs ps => ps.status.displayName,
+ final DockerPs ps => ps.state ?? ps.status.displayName,
}}',
style: UIs.text13Grey,
</code_context>
<issue_to_address>
Fallback to displayName may cause inconsistent UI if state is not localized.
Ensure both ps.state and ps.status.displayName are consistently localized to avoid mismatched or untranslated status values in the UI.
Suggested implementation:
```
'${item.image ?? l10n.unknown} - ${switch (item) {
final PodmanPs ps => l10n.localizeStatus(ps.status.displayName),
final DockerPs ps => l10n.localizeStatus(ps.state ?? ps.status.displayName),
}}',
```
If `l10n.localizeStatus` does not exist, you need to implement it in your localization class. It should accept a status string and return the localized version, falling back to the original string if no translation is available. Example:
```dart
// In your l10n class
String localizeStatus(String status) {
switch (status) {
case 'Running':
return running; // already localized
case 'Stopped':
return stopped; // already localized
// Add other status cases as needed
default:
return status;
}
}
```
Make sure to update the localization files if you add new status values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| /// Get display string for the status | ||
| String get displayName { |
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.
suggestion: Hardcoded display names may limit localization support.
Consider using a localization mechanism for displayName to support multiple languages if your app requires internationalization.
Suggested implementation:
/// Get localized display string for the status
String displayName(BuildContext context) {
final localizations = AppLocalizations.of(context);
return switch (this) {
ContainerStatus.running => localizations.containerStatusRunning,
ContainerStatus.exited => localizations.containerStatusExited,
ContainerStatus.created => localizations.containerStatusCreated,
ContainerStatus.paused => localizations.containerStatusPaused,
ContainerStatus.restarting => localizations.containerStatusRestarting,
ContainerStatus.removing => localizations.containerStatusRemoving,
ContainerStatus.dead => localizations.containerStatusDead,
ContainerStatus.unknown => localizations.containerStatusUnknown,
};
}
- You must add the corresponding keys (e.g.,
containerStatusRunning,containerStatusExited, etc.) to yourintlARB files and regenerate your localization classes. - Update all usages of
displayNameto pass aBuildContextparameter, e.g.,status.displayName(context). - Ensure
AppLocalizationsis imported:import 'package:flutter_gen/gen_l10n/app_localizations.dart';
| final PodmanPs ps => ps.running ? l10n.running : l10n.stopped, | ||
| final DockerPs ps => ps.state, | ||
| final PodmanPs ps => ps.status.displayName, | ||
| final DockerPs ps => ps.state ?? ps.status.displayName, |
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.
suggestion: Fallback to displayName may cause inconsistent UI if state is not localized.
Ensure both ps.state and ps.status.displayName are consistently localized to avoid mismatched or untranslated status values in the UI.
Suggested implementation:
'${item.image ?? l10n.unknown} - ${switch (item) {
final PodmanPs ps => l10n.localizeStatus(ps.status.displayName),
final DockerPs ps => l10n.localizeStatus(ps.state ?? ps.status.displayName),
}}',
If l10n.localizeStatus does not exist, you need to implement it in your localization class. It should accept a status string and return the localized version, falling back to the original string if no translation is available. Example:
// In your l10n class
String localizeStatus(String status) {
switch (status) {
case 'Running':
return running; // already localized
case 'Stopped':
return stopped; // already localized
// Add other status cases as needed
default:
return status;
}
}Make sure to update the localization files if you add new status values.
Fixes #795
Summary by Sourcery
Introduce a ContainerStatus enum to standardize container state handling, refactor existing DockerPs and PodmanPs models to use status instead of a running flag, update UI components to rely on status properties, and add comprehensive tests for status parsing and utility methods.
Enhancements:
Tests: