-
Notifications
You must be signed in to change notification settings - Fork 47
docs: document 28 of the existing rust community modules #142
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: main
Are you sure you want to change the base?
docs: document 28 of the existing rust community modules #142
Conversation
modules/postgresql/index.md
Outdated
|
||
let postgres_instance = postgres::Postgres::default().start().unwrap(); | ||
|
||
let connection_string = format!( |
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: here in the catalog, we usually just add the container instance declaration, avoiding the addition of more in depth info on how to connect to them. We delegate that to the final docs for each language
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.
fair enough. I just copied the examples from docs.rs ^^
Not quite sure what this means concretely for the modules as noted below 😅
modules/clickhouse/index.md
Outdated
use testcontainers_modules::{clickhouse, testcontainers::runners::SyncRunner}; | ||
|
||
let clickhouse = clickhouse::ClickHouse::default().start().unwrap(); | ||
let http_port = clickhouse.get_host_port_ipv4(8123).unwrap(); | ||
|
||
// do something with the started clickhouse instance.. |
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.
@mdelapenya so that we sync on what you concretely mean:
Should I remove this part?
use testcontainers_modules::{clickhouse, testcontainers::runners::SyncRunner}; | |
let clickhouse = clickhouse::ClickHouse::default().start().unwrap(); | |
let http_port = clickhouse.get_host_port_ipv4(8123).unwrap(); | |
// do something with the started clickhouse instance.. | |
use testcontainers_modules::{clickhouse, testcontainers::runners::SyncRunner}; | |
let clickhouse = clickhouse::ClickHouse::default().start().unwrap(); |
or even the imports
use testcontainers_modules::{clickhouse, testcontainers::runners::SyncRunner}; | |
let clickhouse = clickhouse::ClickHouse::default().start().unwrap(); | |
let http_port = clickhouse.get_host_port_ipv4(8123).unwrap(); | |
// do something with the started clickhouse instance.. | |
let clickhouse = clickhouse::ClickHouse::default().start().unwrap(); |
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.
We are not adding the import in the code snippets, just the var declaration for the container using the module, so I'd go with your second option: is shorter and straight forward, reinforcing the idea that modules are easy to use. Besides, modern IDEs usually autocomplete the imports (Go user here 🙋 😅 ) so it's probably the case in the Rust ecosystem.
Other than that, really appreciate your work adding all these modules to the catalog! The TC Rust community will be very glad for it 🙇
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.
Shure.
In that case, lets also remove the variable assignment.
I would like to keep the originating crate to avoid confusion. Maybe they have a variable named clickhouse if they work with that.
use testcontainers_modules::{clickhouse, testcontainers::runners::SyncRunner}; | |
let clickhouse = clickhouse::ClickHouse::default().start().unwrap(); | |
let http_port = clickhouse.get_host_port_ipv4(8123).unwrap(); | |
// do something with the started clickhouse instance.. | |
testcontainers_modules::clickhouse::ClickHouse::default().start() |
The error message they get should explain the rest good enough.
modules/mssql/index.md
Outdated
maintainer: community | ||
example: | | ||
```rust | ||
use testcontainers_modules::{testcontainers::runners::SyncRunner, mssql_server}; |
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.
this one needs accepting the eula.. lets keep it ^^
url: https://docs.rs/testcontainers-modules/latest/testcontainers_modules/oracle/free/struct.Oracle.html | ||
maintainer: community | ||
example: | | ||
```rust |
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.
lets keep the warning ^^
maintainer: community | ||
example: | | ||
```rust | ||
use testcontainers::core::ImageExt; |
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.
since this does not work otherwise, here I am keeping the imports.
It is otherwise a bit confusing
there are quite a few rust modules missing.
This PR adds a bunch of them
CC @DDtKey