-
Notifications
You must be signed in to change notification settings - Fork 4
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
Prefer using a strategy factory to set the deduplication strategy per… #54
Prefer using a strategy factory to set the deduplication strategy per… #54
Conversation
… ContentMerger. Otherwise one deduplication strategy instance is used for all ContentMerger instances, wich will cause concurrency issues and missing data, if the strategy has state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor naming issues, but not important enough to warrant a merge block. So feel free to merge
@@ -89,7 +89,10 @@ func Test_CompositionHandler_PositiveCaseWithSimpleDeduplicationStrategy(t *test | |||
} | |||
} | |||
ch := NewCompositionHandler(ContentFetcherFactory(contentFetcherFactory)) | |||
ch.WithDeduplicationStrategy(new(SimpleDeduplicationStrategy)) | |||
sdf := func() StylesheetDeduplicationStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why sdf? sds would be more fitting (minor issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stylesheet deduplication (missing strategy) factory :-)
@@ -42,7 +42,10 @@ func compositionHandler() http.Handler { | |||
|
|||
return fetcher | |||
} | |||
return composition.NewCompositionHandler(contentFetcherFactory).WithDeduplicationStrategy(new(composition.SimpleDeduplicationStrategy)) | |||
sdf := func() composition.StylesheetDeduplicationStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, why sdf? (minor issues, naming)
… ContentMerger.
Otherwise one deduplication strategy instance is used for all
ContentMerger instances, wich will cause concurrency issues and missing
data, if the strategy has state.