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

Convert public API to builder pattern #17

Open
mcasper opened this issue Nov 10, 2017 · 4 comments
Open

Convert public API to builder pattern #17

mcasper opened this issue Nov 10, 2017 · 4 comments

Comments

@mcasper
Copy link
Owner

mcasper commented Nov 10, 2017

Trying to provide a clean API that has lots of optional fields is pretty verbose right now, as was made clear by service checks, not to mention all the options that events support that we should add. My thought is to transition the API to more of a builder pattern, so that you never have to specify the optional arguments that you don't care about:

client.service_check("redis.can_connect", ServiceStatus::OK)
    .hostname("my-host.localhost")
    .timestamp(1510327616)
    .send()

or

client.gauge("my_gauge", "12345")
    .tag("special:tag")
    .send()
@dwdking
Copy link
Contributor

dwdking commented Apr 9, 2022

I was thinking the same thing around the Options struct. My current PR that adds default_tags to Options and Client has breaking code changes for anyone bumping the dependency, using a builder pattern here would make future changes a little easier to uptake.

I was thinking something along these lines. If you are in agreement I can either include both changes in the single PR or open the builder after the other one is merged.

@mcasper
Copy link
Owner Author

mcasper commented Apr 13, 2022

I think opening that up as a separate PR would be great!

@dwdking
Copy link
Contributor

dwdking commented Apr 23, 2022

I think opening that up as a separate PR would be great!

Opened the PR.

On an unrelated note is there a road-map or a planned time when you might release a new version of the crate with this and the previous PR that added the default tags?

@mcasper
Copy link
Owner Author

mcasper commented Apr 25, 2022

@dwdking I just merged the options builder PR and released a new version! (0.7.0)

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

No branches or pull requests

2 participants