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

Update asNodeList() on common/Config #6502

Closed
arjav-desai opened this issue Mar 28, 2023 · 2 comments
Closed

Update asNodeList() on common/Config #6502

arjav-desai opened this issue Mar 28, 2023 · 2 comments
Assignees
Labels
config design This issue requires design/architecture decisions Níma Helidon Níma wontfix This will not be worked on
Milestone

Comments

@arjav-desai
Copy link
Member

While the code to change may not be in repo yet common/config/src/main/java/io/helidon/common/config/Config.java

ConfigValue<List> asNodeList() throws ConfigException;

its coming as part of common-config pr #6448 and pico pr as well https://github.com/helidon-io/helidon/pull/5750/files#diff-557d7016d9aceb8cd7b08e48eeb87fe488ba13da5c79a3acabc6ed52d3196acbR228

There is a review comment on first PR i.e. #6448 (comment)

I think that this would allow you to assign the result into a list of a arbitrary implementation of Config, eg.:
ConfigValue<List> myConfig = commonConfig.asNodeList();
Which breaks type safety (as the result is most likely going to be somebody else's implementation of config)
To make this right, we would have to have the C defined on the Config type, but that would create a lot of problems.
I think this must be changed

@arjav-desai arjav-desai added config Níma Helidon Níma labels Mar 28, 2023
@arjav-desai arjav-desai added this to the 4.0.0-M1 milestone Mar 28, 2023
@barchetta barchetta modified the milestones: 4.0.0-ALPHA6, 4.0.0-M2 Apr 4, 2023
@tomas-langer tomas-langer added the design This issue requires design/architecture decisions label Apr 5, 2023
@tomas-langer
Copy link
Member

To correctly fix this issue, we would have to fully separate API and implementation of Config. This would be a huge backward incompatible change.
I am investigating options to make this a bit less impactful. We may actually end up with this "wrong" method to be able to maintain backward compatibility for our users

@barchetta barchetta modified the milestones: 4.0.0-M1, 4.0.0-M2 May 18, 2023
@m0mus m0mus modified the milestones: 4.0.0-M2, 4.x Jul 24, 2023
@tomas-langer
Copy link
Member

I am closing this issue as will not fix.
Although this can create a code that compiles and failes at runtime, you would have to do some custom code to achieve that, which would already require understanding of the problem.
I think we can accept failure in such an edge case, with the benefit of very small impact to our users.

@tomas-langer tomas-langer added the wontfix This will not be worked on label Sep 6, 2023
@m0mus m0mus added this to Backlog Aug 12, 2024
@m0mus m0mus moved this to Closed in Backlog Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config design This issue requires design/architecture decisions Níma Helidon Níma wontfix This will not be worked on
Projects
Archived in project
Development

No branches or pull requests

4 participants