Skip to content
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

Add JetStream types and basics #457

Merged
merged 1 commit into from
May 27, 2022
Merged

Add JetStream types and basics #457

merged 1 commit into from
May 27, 2022

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented May 27, 2022

No description provided.

@Jarema Jarema changed the title JS types and basics Add JetStream types and basics May 27, 2022
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

First pass, bit of bike shedding on our old abbreviations.

async-nats/src/jetstream/context.rs Outdated Show resolved Hide resolved
async-nats/src/jetstream/context.rs Outdated Show resolved Hide resolved
skip_serializing_if = "is_default"
)]
/// Indicates if rollups will be allowed or not.
pub allow_rollup: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we wanna be explicit?

Suggested change
pub allow_rollup: bool,
pub allow_rollup_headers: bool,

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I would stick to name describing behaviour.

async-nats/src/jetstream/types.rs Outdated Show resolved Hide resolved
async-nats/src/jetstream/types.rs Outdated Show resolved Hide resolved
async-nats/src/jetstream/types.rs Outdated Show resolved Hide resolved
@@ -15,6 +15,7 @@ use crate::Client;

pub mod context;
pub mod response;
pub mod types;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets scope this to jetstream streams, we'll spot any duplication easily.

Suggested change
pub mod types;
pub mod stream;

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed the whole file. makes more sense.

StreamConfig: From<S>,
{
let config: StreamConfig = stream_config.into();
if config.name.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets ditch this check and add a test for it, ensure the server does still error.

Suggested change
if config.name.is_empty() {

Copy link
Member Author

Choose a reason for hiding this comment

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

as found out, it returns no responders. Im still all for server-checks, but in this case... I would have this in.

async-nats/src/jetstream/context.rs Outdated Show resolved Hide resolved
@@ -49,4 +54,25 @@ impl Context {

Ok(response)
}

pub async fn add_stream<S>(&mut self, stream_config: S) -> Result<StreamInfo, Error>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add versus create? endpoint is create.

Suggested change
pub async fn add_stream<S>(&mut self, stream_config: S) -> Result<StreamInfo, Error>
pub async fn create_stream<S>(&mut self, stream_config: S) -> Result<StreamInfo, Error>

Copy link
Member Author

Choose a reason for hiding this comment

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

and all APIs are add.... hm.

@Jarema Jarema force-pushed the jarema/js-api branch 3 times, most recently from 9e23a74 to e7f49cc Compare May 27, 2022 12:31
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

lgtm

@Jarema Jarema merged commit caadcf3 into main May 27, 2022
@Jarema Jarema deleted the jarema/js-api branch May 27, 2022 13:01
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.

2 participants