Navigation Menu

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

ConfigProperty default name #233

Closed
Emily-Jiang opened this issue Sep 4, 2017 · 10 comments
Closed

ConfigProperty default name #233

Emily-Jiang opened this issue Sep 4, 2017 · 10 comments
Milestone

Comments

@Emily-Jiang
Copy link
Member

In ConfigProperty.java, the javadoc says
{@code class_name} is the fully qualified name of the class being injected to with the first letter decaptialised.

Originally, I proposed to be {@code class_name} is the simple name of the class being injected to with the first letter decaptialised.
The decaptialised bit does make sense.

Later on, Mark changed it to be fully qualified classname. I missed the commit. The decaptialised bit looks very odd. e.g
com.acme.myBean.myProperty
as com.acme.Mybean.myProperty is much better read.

Can we remove the decaptialisation bit? I know this is behaviour change. Considering the light usage, it should be minimal. Thoughts?

@Emily-Jiang
Copy link
Member Author

Emily-Jiang commented Sep 4, 2017

@struberg @OndrejM @johnament should this be fix this in MP Config 1.1?

@Emily-Jiang
Copy link
Member Author

I'll vote yes, though.

@OndroMih
Copy link
Contributor

OndroMih commented Sep 4, 2017

This would be a breaking change, although this is how I wanted it initially. And to be honest, I was really confused why our Payara impl decapitalizes the class name until I found out that this is specified by the spec.

I'm in favor of this fix, although we may also preserve the current behavior as deprecated and the new behavior taking precedence if both keys are found. In other words, if a Capitalized config key is found, it will be used, and if not, decapitalized key will be searched for and used if found. The new behavior should be stressed while the old behavior still supported and mentioned somewhere in the spec (e.g. as a compatibility or deprecation note).

@Emily-Jiang
Copy link
Member Author

+1 on Ondrej!

@johnament
Copy link
Contributor

Since there was no TCK test for this previously, I'm OK if we change the behavior in the release. GConfig actually didn't support it at all.

@johnament
Copy link
Contributor

BTW, if you want this for MP Config 1.1, it has to have a pull request submitted today. If it doesn't make this release, we'll need to keep both behaviors around for following releases.

@Emily-Jiang
Copy link
Member Author

Emily-Jiang commented Sep 5, 2017

ok. can we agree on which one to go for? I will submit a PR once a decision is made.

  1. Simplified classname with the first letter decaptialised
  2. Fully qualified classname?
  3. Support as Ondro suggested: both fully qualified classname and fully qualified classname with the first letter decapitialised
    At this stage, I am fine with either 1 or 2. Most runtime will go directly to Config 1.1. Can we vote and then I will do a PR?

@smillidge
Copy link
Contributor

2 or 2.toLowerCase()

@Emily-Jiang
Copy link
Member Author

Thanks @smillidge ! I'll provide a PR with 2.

Emily-Jiang added a commit that referenced this issue Sep 5, 2017
Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
@OndroMih
Copy link
Contributor

OndroMih commented Sep 5, 2017

I'm in favor of 2 for the sake of simplicity. I agree that there's not much risk in introducing this breaking change in such a short time, provided that it's properly announced (e.g. in release notes).

Emily-Jiang added a commit that referenced this issue Sep 5, 2017
Emily-Jiang added a commit that referenced this issue Sep 5, 2017
Emily-Jiang added a commit that referenced this issue Sep 5, 2017
Emily-Jiang added a commit that referenced this issue Sep 5, 2017
Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
johnament pushed a commit that referenced this issue Sep 6, 2017
* #233 configProperty default name

Signed-off-by: Emily Jiang <emijiang6@googlemail.com>

* emily_config1.1

Signed-off-by: Emily Jiang <emijiang6@googlemail.com>

* Revert "#233 configProperty default name"

This reverts commit 7830f57.

* Revert "emily_config1.1"

This reverts commit 6caf28b.

* Revert "Revert "#233 configProperty default name""

This reverts commit 5139429.

* Revert "#233 configProperty default name"

This reverts commit 7830f57.

* #233 configProperty default name

Signed-off-by: Emily Jiang <emijiang6@googlemail.com>
@johnament johnament added this to the MP Config 1.1 milestone Sep 6, 2017
johnament added a commit that referenced this issue Sep 6, 2017
Signed-off-by: John D. Ament <john.d.ament@gmail.com>
johnament added a commit that referenced this issue Sep 6, 2017
Signed-off-by: John D. Ament <john.d.ament@gmail.com>
johnament added a commit that referenced this issue Sep 6, 2017
Signed-off-by: John D. Ament <john.d.ament@gmail.com>
johnament added a commit that referenced this issue Sep 7, 2017
)

Signed-off-by: John D. Ament <john.d.ament@gmail.com>
starksm64 added a commit to starksm64/wildfly-microprofile-config that referenced this issue Sep 16, 2017
…derived

name from a class
eclipse/microprofile-config#233

Signed-off-by: Scott Stark <starksm64@gmail.com>
jmesnil pushed a commit to jmesnil/smallrye-config that referenced this issue May 23, 2018
…default derived

name from a class
eclipse/microprofile-config#233

Signed-off-by: Scott Stark <starksm64@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants