abderrahim commented on PR #2119: URL: https://github.com/apache/buildstream/pull/2119#issuecomment-4412204343
I'm not sure what to think of this. The implementation and the reasoning behind it look correct, but I'm not a fan of adding a callback like this. I'm not very knowledgeable about the loading process, so correct me if my understanding is wrong. The usual solution for this kind of things that we want to use at load time is to load them early, so that we can have them already when loading junctions. But for this particular case, we want to avoid that to allow these source provenance attributes to be loaded using an include? I wonder if it wouldn't be nicer to have some regular Plugin method that gets called on second pass, rather than having to register a callback. Conceptually, I think this check needs to happen at plugin configure time rather than at plugin load time. Maybe we can do that instead by having `Source` override `Plugin._configure()` to make this check? Sorry if I don't sound coherent, I wrote this as I'm trying to understand and didn't edit it afterwards because I wanted to document my chain of thoughts. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
