fix(enricher): Using wrong configuration when many enrichers exist#81
fix(enricher): Using wrong configuration when many enrichers exist#81simonprovost merged 1 commit intomainfrom
Conversation
simonprovost
left a comment
There was a problem hiding this comment.
Amazing @fabiofelix! Good catch. I’m approving the PR 🎉
Just a quick one in case you find it interesting. Would you believe it would be nice to add a unit test to check if the config is properly set with two loaders during enriching? This way, it’ll be caught if it’s removed by any means later on.
I’m not requesting the change but rather let you decide so, but if it’s all good, let me merge today, please @ me :)
Cheers
|
Yes @simonprovost, we could create an issue to add this type of test, not just for |
|
@soniacq it was a very tiny PR so I took the liberty to merge without second review, hope it's all fine ✅ |
|
Thanks a bunch @fabiofelix ! ⭐ |
EnricherFactoryfeeds enrichers with the sameconfigreference, which causes wrong functioning.For example, if an
UrbanPipelinehas more than oneloaderand more than oneenricher, all the enrichers will share the same configuration - the last one defined in theUrbanPipeline.With the present modification,
EnricherFactorysends a copy of its internal configuration to eachenricher. This way, eachenricherhas a different configuration.📚 Documentation preview 📚: https://UrbanMapper--81.org.readthedocs.build/en/81/