Skip to content

feat: Update readme #26

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Update readme #26

wants to merge 1 commit into from

Conversation

dotKuro
Copy link
Member

@dotKuro dotKuro commented Jun 12, 2025

No description provided.

@dotKuro dotKuro requested a review from a team as a code owner June 12, 2025 05:45
@goloroden goloroden self-assigned this Jun 12, 2025
Copy link
Contributor

@Snapstromegon Snapstromegon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't repeat comments all over the place.
We should probably consider splitting README.md and the crate entry docs.

Also we should add a section on compatibility with the cloudevents-sdk crate.

Right now the code in the examples feels very "go-like" and I added some comments on how to make them more "rusty".

Comment on lines +35 to +37
if result.is_err() {
// ...
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method of error handling is strongly discouraged in Rust, since you need to extract the Err or Ok type anyways.
This type of error handling is "very go-like" and for Rust I'd recommend using the ? operator or .expect("Reason") in examples instead as it is more concise and more "rusty".

Then call the `ping` function to check whether the instance is reachable. If it is not, the function will return an error:

```rust
let result = client.ping();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote in the issue, this file will be run as part of tests with each rust codeblock being its own test right now (this is also why QA is failing).
We either need to separate the two (which would probably be better IMO) and move the "real" examples into the lib.rs file where they can be checked and lines of code can be hidden or make the examples valid here (so e.g. by setting up a testcontainer DB).

let client = Client::new(base_url, api_token);
```

Then call the `ping` function to check whether the instance is reachable. If it is not, the function will return an error:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping -> [client::Client::ping] for RustDoc (so it links). This would also not be necessary when separating the two docs parts.


*Note that `Ping` does not require authentication, so the call may succeed even if the API token is invalid.*

If you want to verify the API token, call `verify_api_token`. If the token is invalid, the function will return an error:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as with ping

Comment on lines +45 to +48
let result = client.verify_api_token();
if result.is_err() {
// ...
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has to be valid Rust example right now - see above

"author": "Arthur C. Clarke",
"isbn": "978-0756906788",
}))
.build()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing ;

Comment on lines +74 to +77
match result {
Ok(written_events) => // ...
Err(err) => // ...
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here I recommend doing ?

Comment on lines +284 to +287
ObserveEventsRequestOptions {
recursive: true,
..Default::default(),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Options do not yet implement Default (#27 tracks implementing it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants