-
Notifications
You must be signed in to change notification settings - Fork 249
Remove unused module ContainerSpecHelper #1563
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
base: main
Are you sure you want to change the base?
Conversation
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.
This will be used by the NoSQL work.
I don’t think it makes sense to create a standalone module just for a potential future persistence implementation. Even if this class is needed for NoSQL support, it should live within the NoSQL module itself, not in a separate module that adds unnecessary structure and overhead. Let’s keep the module boundaries meaningful and aligned with actual usage. |
Will it only be used by the NoSQL work? If not, maybe it can live outside the nosql module, but if so it should probably be contained there. |
Use it for Minio, Keycloak, Postgres, etc etc etc. |
Just to clarify, Postgres doesn't use it, and Minio and Keycloak are not part of the Polaris repo. |
TBH, I don't understand why you want to remove something that's known to be used by work that supposed to go in. We do add "dangling" pieces for upcoming work all the time. I don't understand why you're pushing to remove exactly this part. |
This isn’t just another piece of harmless scaffolding. It introduces public API. Any public class becomes part of our compatibility contract. Once it lands, removing or reshaping it is a breaking change. That means we should only ship it when we’re confident in the design and need. Dead code isn’t free.
|
I don't think I've personally ever added a dangling module for upcoming work, and I've scarcely seen it done in a way that wasn't super-specific to the upcoming work (e.g. a JDBC module for the upcoming JDBC work). We should try to minimize this, if possible. |
It isn't used anywhere in the project, plus it causes potential extra maintenance cost like #1562.