search

nats-io / nats.rs

Rust client for NATS, the cloud native messaging system.
Apache License 2.0
1.04k stars 165 forks source link

De/Serialize `sample_frequency` correctly for Push and Pull Consumers #1300

Closed bengsparks closed 1 month ago

bengsparks commented 1 month ago

Closes #1299

This PR instructs serde to rename sample_frequency into sample_freq during de/serialization. It also introduces a module for converting the u8 into a String due to nats-server's handling of the SampleFrequency field.

The PR is split into two separate commits, one for async-nats and the other for nats, so that the latter can be reverted if it is not wanted.

This PR does not contain any tests, but used the setup from #1299 to check if the field was being correctly set.

caspervonb commented 1 month ago

Looks good to me, as a test one could confirm that the config is reported back as expected. Maybe split the nats change into its own PR for changelog generation.

bengsparks commented 1 month ago

@caspervonb The PR now includes a test in async-nats/tests/jetstream_tests.rs that can be executed via cargo test jetstream::consumer_configs_sample_frequency, which checks that the cached info of push and pull consumers now contain the correctly set field. If this PR is given the go-ahead, then I'll revert the commit for the nats module and open a second PR.

bengsparks commented 1 month ago

@Jarema I've reverted the commit that uses the nats module. I had to fumble around with the commit history a bit (might've accidentally included a commit from another contributor?), but this should be fine to merge (when squashed).