You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by "kwin (via GitHub)" <gi...@apache.org> on 2024/11/15 14:35:27 UTC
[PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
kwin opened a new pull request, #345:
URL: https://github.com/apache/felix-dev/pull/345
(no comment)
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #345:
URL: https://github.com/apache/felix-dev/pull/345#issuecomment-2479093563
I just aligned with the logic from https://github.com/apache/felix-dev/blob/958c28de32db1ec062b273b4c0e5faa9a70a87b6/configadmin/src/main/java/org/apache/felix/cm/file/ConfigurationHandler.java#L210. But I am also fine with making this an opt-in behaviour.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #345:
URL: https://github.com/apache/felix-dev/pull/345#discussion_r1843926061
##########
cm.json/src/main/java/org/apache/felix/cm/json/io/package-info.java:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-@Version("1.0.0")
+@Version("1.1.0")
Review Comment:
I would like to increase the minor version here in order to ease depending on this fixed version.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #345:
URL: https://github.com/apache/felix-dev/pull/345#discussion_r1844203094
##########
cm.json/src/main/java/org/apache/felix/cm/json/io/package-info.java:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-@Version("1.0.0")
+@Version("1.1.0")
Review Comment:
This is a potentially backward incompatible semantical change (affecting consumers). Just a change which is not detected by baselining because the method signature did not change. Therefore adjusting the minor version is IMHO the right thing to do!
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "laeubi (via GitHub)" <gi...@apache.org>.
laeubi commented on code in PR #345:
URL: https://github.com/apache/felix-dev/pull/345#discussion_r1844028819
##########
cm.json/src/main/java/org/apache/felix/cm/json/io/package-info.java:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-@Version("1.0.0")
+@Version("1.1.0")
Review Comment:
As you only changed internal behavior, package versions are not for "easing" anything but to allow semantic versioning so increasing the minor is wrong here.
```suggestion
@Version("1.0.1")
```
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #345:
URL: https://github.com/apache/felix-dev/pull/345#issuecomment-2479030941
@cziegeler Please have a look, this test fails without sorting the keys first.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #345:
URL: https://github.com/apache/felix-dev/pull/345#issuecomment-2479088673
It is expected that the tests fail, the configuration object preserves insertion order - if you read and write the same feature without changes, it should be identical (maybe minus whitespaces, indention).
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "laeubi (via GitHub)" <gi...@apache.org>.
laeubi commented on code in PR #345:
URL: https://github.com/apache/felix-dev/pull/345#discussion_r1844209944
##########
cm.json/src/main/java/org/apache/felix/cm/json/io/package-info.java:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-@Version("1.0.0")
+@Version("1.1.0")
Review Comment:
If it affects consumers you need a major version bump.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #345:
URL: https://github.com/apache/felix-dev/pull/345#issuecomment-2479110832
ConfigurationHandler is not really a public API. The configuration writer is used for the various feature model implementations and it would be pretty annoying if it would destroy out of the sudden the order of things. So yes, opt-in makes sense to me.
A different option is to have a utility method that has a Dictionary as an input and outputs a sorted Dictionary. This could then be used everywhere, not just when writing out configurations.
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6736 Emit properties in alphabetical order [felix-dev]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #345:
URL: https://github.com/apache/felix-dev/pull/345#discussion_r1844203094
##########
cm.json/src/main/java/org/apache/felix/cm/json/io/package-info.java:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-@Version("1.0.0")
+@Version("1.1.0")
Review Comment:
This is a potentially backward semantical change (affecting consumers). Just a change which is not detected by baselining because the method signature did not change. Therefore adjusting the minor version is IMHO the right thing to do!
--
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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org