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

refactor!: require the builder pattern for module initalisation #161

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

CommanderStorm
Copy link
Contributor

@CommanderStorm CommanderStorm commented Jul 7, 2024

This PR makes sure that the way structs are constructed is consistent and encourages the builder pattern.
It is based on this question: #156 (comment)

For the changelog:

We now require you to use the builder pattern (instead of some structs being unit structs) for all modules.
This ensures that if we add a field in the future that this will not break your existing code.

This change is breaking for these modules:

Module before after
cncf_distribution::CncfDistribution CncfDistribution.start() CncfDistribution::default().start()
dynamodb_local::DynamoDb DynamoDb.start() DynamoDb::default().start()
elasticmq::ElasticMq ElasticMq.start() ElasticMq::default().start()
mongo::Mongo (see #143) Mongo.start() Mongo::default().start()
kwok::KwokCluster KwokCluster.start() KwokCluster::default().start()
rabbitmq::RabbitMq RabbitMq.start() RabbitMq::default().start()
redis::stack::RedisStack RedisStack.start() RedisStack::default().start()
redis::standalone::Redis Redis.start() Redis::default().start()
victoria_metrics::VictoriaMetrics VictoriaMetrics.start() VictoriaMetrics::default().start()

@CommanderStorm CommanderStorm force-pushed the consistent-struct-initialisation branch from e4e0a17 to 6045cb7 Compare July 7, 2024 16:47
Copy link
Contributor

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

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

I think we can use refactor instead of chore here

Thank you for the PR! ❤️

@CommanderStorm CommanderStorm changed the title chore!: require the builder pattern for module initalisation refactor!: require the builder pattern for module initalisation Jul 7, 2024
@CommanderStorm CommanderStorm force-pushed the consistent-struct-initialisation branch 2 times, most recently from 596faf7 to ed7a50b Compare July 7, 2024 18:28
@CommanderStorm
Copy link
Contributor Author

Let's clean this up (rustfmt & etc)

Sorry for that part. Forgot to do rustfmt after doing the (apparently missing doctests) testing run

@CommanderStorm
Copy link
Contributor Author

I don't know why the test for mssql_server added in #72 is failing.

Cannot debug this locally.
@kymmt90 I am assuming you are using this in your testing infra. Is this flaky for you too?

@DDtKey
Copy link
Contributor

DDtKey commented Jul 7, 2024

It's definitely not related to this PR and seems flaky - yes. For some reason, I think it happens only on nightly channel (might be coincidence though) 🤔

Need to address this but in separate PR for sure

@CommanderStorm CommanderStorm force-pushed the consistent-struct-initialisation branch from ed7a50b to 54c2d9f Compare July 20, 2024 19:57
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