Conversation
Reviewer's GuideRefactors DockerContainer and Network to use safer nil handling, nested error types, per-class logging, explicit Nil return annotations, and custom inspect output, while wiring Ameba linting into CI and documenting new style guidance. Class diagram for the updated Testcontainers error hierarchyclassDiagram
class TestcontainersError
class ConnectionError
class NotFoundError
class DockerContainer
class DockerContainer_NotStartedError
class DockerContainer_LaunchError
class DockerContainer_PortNotMappedError
class DockerContainer_HealthcheckNotSupportedError
class DockerContainer_TimeoutError
class Network
class Network_Error
class Network_NotFoundError
class Network_AlreadyExistsError
class ContainerNotStartedError
class ContainerLaunchError
class PortNotMappedError
class HealthcheckNotSupportedError
class TimeoutError
class NetworkError
class NetworkNotFoundError
class NetworkAlreadyExistsError
TestcontainersError <|-- ConnectionError
TestcontainersError <|-- NotFoundError
TestcontainersError <|-- DockerContainer_NotStartedError
TestcontainersError <|-- DockerContainer_LaunchError
TestcontainersError <|-- DockerContainer_PortNotMappedError
TestcontainersError <|-- DockerContainer_HealthcheckNotSupportedError
TestcontainersError <|-- DockerContainer_TimeoutError
TestcontainersError <|-- Network_Error
Network_Error <|-- Network_NotFoundError
Network_Error <|-- Network_AlreadyExistsError
DockerContainer *-- DockerContainer_NotStartedError
DockerContainer *-- DockerContainer_LaunchError
DockerContainer *-- DockerContainer_PortNotMappedError
DockerContainer *-- DockerContainer_HealthcheckNotSupportedError
DockerContainer *-- DockerContainer_TimeoutError
Network *-- Network_Error
Network *-- Network_NotFoundError
Network *-- Network_AlreadyExistsError
class ContainerNotStartedError {
<<alias>>
}
class ContainerLaunchError {
<<alias>>
}
class PortNotMappedError {
<<alias>>
}
class HealthcheckNotSupportedError {
<<alias>>
}
class TimeoutError {
<<alias>>
}
class NetworkError {
<<alias>>
}
class NetworkNotFoundError {
<<alias>>
}
class NetworkAlreadyExistsError {
<<alias>>
}
ContainerNotStartedError ..> DockerContainer_NotStartedError
ContainerLaunchError ..> DockerContainer_LaunchError
PortNotMappedError ..> DockerContainer_PortNotMappedError
HealthcheckNotSupportedError ..> DockerContainer_HealthcheckNotSupportedError
TimeoutError ..> DockerContainer_TimeoutError
NetworkError ..> Network_Error
NetworkNotFoundError ..> Network_NotFoundError
NetworkAlreadyExistsError ..> Network_AlreadyExistsError
Class diagram for DockerContainer and Network structural updatesclassDiagram
class DockerContainer {
+String? name
+String image
+String? container_id
+Time? created_at
+Hash(String,String) labels
+Hash(String,Hash(String,String)) exposed_ports
+Hash(String,Array~Docr_Types_PortBinding~) port_bindings
+String host()
+Int32 mapped_port(port : Int32|String)
+Int32 first_mapped_port()
+Bool running?()
+Bool healthy?()
+Bool supports_healthcheck?()
+Bool exists?()
+Docr_Types_ContainerInspectResponse info()
+String logs(stdout : Bool, stderr : Bool)
+String exec(cmd : Array~String~)
+Bool wait_for_logs(matcher : Regex, timeout : Int32, interval : Float64)
+Bool wait_for_tcp_port(port : Int32, timeout : Int32, interval : Float64)
+Bool wait_for_healthcheck(timeout : Int32, interval : Float64)
+Bool wait_for_http(container_port : Int32, path : String, status : Int32, https : Bool)
+self stop(force : Bool)
+self kill(signal : String)
+self remove(force : Bool, volumes : Bool)
+self restart()
+self pause()
+self unpause()
+void inspect(io : IO) : Nil
-String require_container_id() : String
-void add_exposed_port(port : Int32|String) : Nil
-void add_fixed_exposed_port(container_port : Int32|String, host_port : Int32) : Nil
-void add_env(env_str : String) : Nil
-void add_labels(labels : Hash~String,String~) : Nil
<<has constant>> Log
}
class Network {
+String name
+String? network_id
+Hash(String,String) labels
+self create!()
+Bool created?()
+Docr_Types_Network info()
+self remove()
+void inspect(io : IO) : Nil
<<has constant>> Log
}
class DockerClient {
+Docr_Types_API api
+String host()
+void reset!() : Nil
}
class DockerContainer_NotStartedError
class DockerContainer_HealthcheckNotSupportedError
class DockerContainer_PortNotMappedError
class Network_Error
DockerContainer ..> DockerClient : uses
Network ..> DockerClient : uses
DockerContainer ..> DockerContainer_NotStartedError : raises
DockerContainer ..> DockerContainer_HealthcheckNotSupportedError : raises
DockerContainer ..> DockerContainer_PortNotMappedError : raises
Network ..> Network_Error : raises
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR enhances code quality and safety by introducing Ameba linting to CI/CD pipelines, standardizing explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
47-49: Minor ordering inconsistency with release workflow.In
ci.yml, the Ameba linter runs after the formatting check (lines 44-45), while inrelease.yml, Ameba runs before the formatting check. Consider aligning the step order for consistency, though both orderings work correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 47 - 49, Reorder the steps in ci.yml so the "Run Ameba linter" step runs before the formatting check to match release.yml; locate the step labeled "Run Ameba linter" (currently running bin/ameba) and move it above the formatting check step so the step order is consistent across workflows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 92: Fix the typographical error in the text snippet "pattern : Nil" by
removing the extra space so it reads "pattern : Nil" (i.e., single space before
the colon); locate the occurrence of the exact string "pattern : Nil" in
AGENTS.md and replace it with "pattern : Nil".
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 47-49: Reorder the steps in ci.yml so the "Run Ameba linter" step
runs before the formatting check to match release.yml; locate the step labeled
"Run Ameba linter" (currently running bin/ameba) and move it above the
formatting check step so the step order is consistent across workflows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b99cd70-8f5c-40b7-8d4d-436696cc13bc
⛔ Files ignored due to path filters (3)
.DS_Storeis excluded by!**/.DS_Store.github/.DS_Storeis excluded by!**/.DS_Storesrc/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (14)
.ameba.yml.github/workflows/ci.yml.github/workflows/release.ymlAGENTS.mdsrc/testcontainers/containers/elasticsearch.crsrc/testcontainers/containers/mariadb.crsrc/testcontainers/containers/mongo.crsrc/testcontainers/containers/mysql.crsrc/testcontainers/containers/postgres.crsrc/testcontainers/containers/rabbitmq.crsrc/testcontainers/docker_client.crsrc/testcontainers/docker_container.crsrc/testcontainers/errors.crsrc/testcontainers/network.cr
| - Use `begin/ensure` blocks for resource cleanup. | ||
| - Keep Docker endpoint configuration centralized via `Testcontainers::DockerClient`. | ||
| - When adding monkey-patches to `docr` types, isolate them in `patches.cr` with clear comments. | ||
| - Use Nil return type annotations pattern : Nil return types on methods that don't return meaningful values (e.g., def max_connections=(value : Int32) : Nil) |
There was a problem hiding this comment.
Typographical error: extra spaces before the colon.
There are two spaces before the colon in "pattern : Nil".
✏️ Proposed fix
-- Use Nil return type annotations pattern : Nil return types on methods that don't return meaningful values (e.g., def max_connections=(value : Int32) : Nil)
+- Use Nil return type annotations pattern: Nil return types on methods that don't return meaningful values (e.g., `def max_connections=(value : Int32) : Nil`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Use Nil return type annotations pattern : Nil return types on methods that don't return meaningful values (e.g., def max_connections=(value : Int32) : Nil) | |
| - Use Nil return type annotations pattern: Nil return types on methods that don't return meaningful values (e.g., `def max_connections=(value : Int32) : Nil`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 92, Fix the typographical error in the text snippet
"pattern : Nil" by removing the extra space so it reads "pattern : Nil" (i.e.,
single space before the colon); locate the occurrence of the exact string
"pattern : Nil" in AGENTS.md and replace it with "pattern : Nil".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3 +/- ##
==========================================
+ Coverage 59.35% 61.14% +1.78%
==========================================
Files 14 14
Lines 342 350 +8
==========================================
+ Hits 203 214 +11
+ Misses 139 136 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adopt LavinMQ-Inspired Crystal Coding Practices
Summary
Applies idiomatic Crystal coding patterns inspired by CloudAMQP's LavinMQ to improve code safety, readability, and debuggability across the library.
Changes
1. Eliminate all
not_nil!calls from source codeReplaced all 18
not_nil!calls insrc/with safe alternatives:require_container_id : Stringprivate helper onDockerContainerthat returns the container ID or raisesNotStartedError— replacing the repeatedraise ... unless @container_id/@container_id.not_nil!pattern.if id = @variableguard clauses inNetworkfor safe nil unwrapping..ameba.ymlto remove source exclusions forLint/NotNil(onlyspec/is excluded now).2. Nested error classes under parent domain
Restructured the flat error hierarchy to nest errors under their owning class, following the LavinMQ pattern of
Error::Subtype:Testcontainers::ContainerNotStartedErrorTestcontainers::DockerContainer::NotStartedErrorTestcontainers::ContainerLaunchErrorTestcontainers::DockerContainer::LaunchErrorTestcontainers::PortNotMappedErrorTestcontainers::DockerContainer::PortNotMappedErrorTestcontainers::HealthcheckNotSupportedErrorTestcontainers::DockerContainer::HealthcheckNotSupportedErrorTestcontainers::TimeoutErrorTestcontainers::DockerContainer::TimeoutErrorTestcontainers::NetworkErrorTestcontainers::Network::ErrorTestcontainers::NetworkNotFoundErrorTestcontainers::Network::NotFoundErrorTestcontainers::NetworkAlreadyExistsErrorTestcontainers::Network::AlreadyExistsErrorBackward-compatible aliases are provided at the old locations so existing user code continues to work.
3. Per-class
Log.forscoped loggingReplaced the single global
Testcontainers.loggerwith per-class log sources:DockerContainer:Log = Testcontainers::Log.for("container")→ source:testcontainers.containerNetwork:Log = Testcontainers::Log.for("network")→ source:testcontainers.networkAll 9
Testcontainers.logger.infocalls replaced withLog.info. TheTestcontainers.loggerclass property is kept for backward compatibility.4.
: Nilreturn type annotationsAdded explicit
: Nilreturn types to 10 methods that don't return meaningful values, following the LavinMQ convention:configure_envon all 6 container modules (Elasticsearch, MariaDB, Mongo, MySQL, Postgres, RabbitMQ)add_exposed_port,add_fixed_exposed_port,add_env,add_labelsonDockerContainerDockerClient.reset!5.
#inspect(io : IO) : NiloverridesAdded structured
#inspectoverrides toDockerContainerandNetworkfor cleaner REPL/debug output instead of dumping all instance variables.6. Ameba linting in CI
Added
bin/amebastep to bothci.ymlandrelease.ymlpipelines.Testing
crystal tool format: cleanSummary by Sourcery
Adopt more idiomatic, safe Crystal patterns across Docker container and network handling, error modeling, logging, and CI linting.
Enhancements:
not_nil!usage in Docker container and network operations with explicit container/network ID checks and a shared helper on DockerContainer.Testcontainers::Log.for, replacing the previous global logger usage.inspectimplementations for DockerContainer and Network to provide concise, targeted debug representations.: Nilreturn types in container modules, DockerContainer helpers, and DockerClient.reset! for clearer intent.CI:
not_nil!usages are removed.Documentation:
not_nil!and preferring explicitNilreturn types.Summary by CodeRabbit
New Features
Refactor
Documentation
Chores