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

Enable an existing WebTracingConfiguration to be used to initialise a… #34

Merged
merged 4 commits into from
Aug 14, 2017

Conversation

objectiser
Copy link
Contributor

… new builder, used to update/replace the previous values

The BeanPostProcessor approach discussed in #28 works, but there is currently no way to update the skip pattern in the WebTracingConfiguration. This PR provides a using method on the builder to enable an existing configuration to be used to initialise the config before the other builder methods are used to change the values.

Its not an issue currently, as the WebTracingConfiguration only has one property - but in the future it might cause issues if (for example) the metrics lib just returns a new config with the pattern but any other properties on the config are not copied.

If you have other thoughts on how to do this then happy to change.

Copy link
Collaborator

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM, although nicer looks toBuilder approach. I think it's also more used:

builder = config.toBuilder()

@objectiser
Copy link
Contributor Author

Updated to use the toBuilder approach.

public static class Builder {
private Pattern skipPattern;

public Builder() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's better to restrict to static Config.builder() only

@@ -22,9 +22,20 @@ public static Builder builder() {
return new Builder();
}

public Builder toBuilder() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also document that this is safe to use only with bean post processing (or maybe some similar approaches) because config values are propagated to different parts.

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