You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Udo Kohlmeyer <ud...@vmware.com> on 2021/10/11 23:33:15 UTC
Geode-9621: Spotless overstepping its boundary of formatter
Hi there Geode Devs,
Recently GEODE-9621<https://issues.apache.org/jira/projects/GEODE/issues/GEODE-9621> was introduced onto the develop branch. This new feature introduces a new bug where `spotlessApply` will remove all version dependencies out of gradle. I assume this is to combat the rising number of cases where developers have been hard-coding versions into the gradle files, rather than using the constraints resolution.
Whilst I like this feature, it also has its draw backs. There is no way to turn off this feature WHEN version overrides are required. An example would be a test where one wants to specifically test a specific version of a library. In my case, this is Spring 4.3.5. My test is designed to deploy a version of Spring, onto Geode, and make sure that the versions of the libraries don’t conflict with one another.
We’ve all agreed to spotless when the code has not conformed to the coding standards that we have all agreed to. To automagically apply the code formatting, using `spotlessApply` is an accepted norm, which each of us follow. But the one thing that I cannot accept, and I hope many of you could not do this either, is when spotless where to actually CHANGE our code. Apply some benign rule that could negatively impact the code behavior.
When we voted for the use of spotless, it was as a code formatter. To make sure that our code base has the same formatting. Seemingly, with GEODE-9621, spotless now have the ability to change the code. I can accept that gradle files will be formatted in specific manner, but I cannot accept that the formatter now has to the power to negatively impact behavior.
Currently there is no way to exclude certain code blocks from the malicious code formatter. Therefore, my tests that require a specific hardcoded version of Spring to be used, will always either fail the validation or tests will fail due to spotless changing my testing scenario by affecting the version of the library that we have to use.
Does anyone have any suggestions as to how we can fix this. My vote is to remove this behavior and limit spotless to only formatting. BUT, that is only my view.
--Udo
Re: Geode-9621: Spotless overstepping its boundary of formatter
Posted by Dan Smith <da...@vmware.com>.
>The formatter now throws when there is a naked version-string on the dependency line, as we discussed.
This new behavior seems reasonable to me. And I think this is what you are asking for, Udo? I do agree that spotlessApply shouldn't change any behavior and cause someone to lose part of their changes as a result.
-Dan
________________________________
From: Udo Kohlmeyer <ud...@vmware.com>
Sent: Monday, October 11, 2021 5:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Hi there Robert,
Yes, you did work with me, but the solution to add my custom versions of jars that I want to test with, into the `DependencyConstraints` file just feels like the wrong approach. The prior art you refer to, is a specific version of tomcat we want each module to use. It is no different to specifying the exact artifact version we want for production code. My case is that I don’t want my artifact version dependencies to leak outside of the test module.
I think, the fact that the decision to elevate spotless from cosmetic formatter to actually having a significant impact, was done unilaterally. Please don’t get me wrong, I love the fact that we want to fix the problem where developers are not following good practices. But I think I am digging in here, because I believe we missed an opportunity to get more voices in this decision.
I also firmly believe that spotless is overstepping the “cosmetic” boundary. I think many developers would feel upset if their code were to be changed. If spotless suddenly replaces a for-loop with a stream implementation.
I think it is one thing to decide if gradle should be formatted with 2 spaces vs 4, or when and how lines wrap, and it’s another for spotless to just remove version information and force one consistent version even if the version was specifically overriden.
I wonder if we can maybe reduce the area of effect of this change and really only force the version formatter to activate for the ‘main’ configuration and leave every other configuration type (test, distributedTest, integrationTest, acceptanceTest, upgradeTest) alone.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Tuesday, October 12, 2021 at 10:38 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Currently there is no way to exclude certain code blocks from the malicious code formatter. Therefore, my tests that require a specific hardcoded version of Spring to be used, will always either fail the validation or tests will fail due to spotless changing my testing scenario by affecting the version of the library that we have to use.
Udo,
I have worked with you to accomplish your goal of not automatically “fixing” the inclusion of dependency versions in the build files, as your work case wants to do something special there. The formatter now throws when there is a naked version-string on the dependency line, as we discussed.
If you want to continue to work around this, there are many cases of prior art in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific versions are allowed.
Good luck.
-Robert Houghton
Re: Geode-9621: Spotless overstepping its boundary of formatter
Posted by Udo Kohlmeyer <ud...@vmware.com>.
Thank you Robert for delivering a solution that detects incursions of versions being defined within gradle files, contravening the rules we have set for using BOM constraints But then finding a way to override the rule in the case where one will HAVE to override the version. (for tests ONLY!!!)
The effort is much appreciated.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Wednesday, October 13, 2021 at 8:07 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
I would not call it a “surge” of them, but I perceived an uptick in them.
From: Udo Kohlmeyer <ud...@vmware.com>
Date: Tuesday, October 12, 2021 at 1:00 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
I had never explicitly asked this, but what was the main driver for this ticket? Was there a surge in explicit version definitions in gradle files? I mean we as committers have agreed to using BOM constraints.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Wednesday, October 13, 2021 at 4:32 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Again, Spotless is not changing code. That was a bad change, as you pointed out, and we modified Spotless to throw an exception instead of stripping out the version string indiscriminately.
We surely could make Spotless only apply to the main source set. The cost would be that tests could use different versions than production. That tests could themselves specify multiple versions at once, and that tests would be out-of-sync with each other, and with the production code. So I would strongly vote against that.
-Robert
From: Udo Kohlmeyer <ud...@vmware.com>
Date: Monday, October 11, 2021 at 5:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Hi there Robert,
Yes, you did work with me, but the solution to add my custom versions of jars that I want to test with, into the `DependencyConstraints` file just feels like the wrong approach. The prior art you refer to, is a specific version of tomcat we want each module to use. It is no different to specifying the exact artifact version we want for production code. My case is that I don’t want my artifact version dependencies to leak outside of the test module.
I think, the fact that the decision to elevate spotless from cosmetic formatter to actually having a significant impact, was done unilaterally. Please don’t get me wrong, I love the fact that we want to fix the problem where developers are not following good practices. But I think I am digging in here, because I believe we missed an opportunity to get more voices in this decision.
I also firmly believe that spotless is overstepping the “cosmetic” boundary. I think many developers would feel upset if their code were to be changed. If spotless suddenly replaces a for-loop with a stream implementation.
I think it is one thing to decide if gradle should be formatted with 2 spaces vs 4, or when and how lines wrap, and it’s another for spotless to just remove version information and force one consistent version even if the version was specifically overriden.
I wonder if we can maybe reduce the area of effect of this change and really only force the version formatter to activate for the ‘main’ configuration and leave every other configuration type (test, distributedTest, integrationTest, acceptanceTest, upgradeTest) alone.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Tuesday, October 12, 2021 at 10:38 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Currently there is no way to exclude certain code blocks from the malicious code formatter. Therefore, my tests that require a specific hardcoded version of Spring to be used, will always either fail the validation or tests will fail due to spotless changing my testing scenario by affecting the version of the library that we have to use.
Udo,
I have worked with you to accomplish your goal of not automatically “fixing” the inclusion of dependency versions in the build files, as your work case wants to do something special there. The formatter now throws when there is a naked version-string on the dependency line, as we discussed.
If you want to continue to work around this, there are many cases of prior art in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific versions are allowed.
Good luck.
-Robert Houghton
Re: Geode-9621: Spotless overstepping its boundary of formatter
Posted by Robert Houghton <rh...@vmware.com>.
I would not call it a “surge” of them, but I perceived an uptick in them.
From: Udo Kohlmeyer <ud...@vmware.com>
Date: Tuesday, October 12, 2021 at 1:00 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
I had never explicitly asked this, but what was the main driver for this ticket? Was there a surge in explicit version definitions in gradle files? I mean we as committers have agreed to using BOM constraints.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Wednesday, October 13, 2021 at 4:32 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Again, Spotless is not changing code. That was a bad change, as you pointed out, and we modified Spotless to throw an exception instead of stripping out the version string indiscriminately.
We surely could make Spotless only apply to the main source set. The cost would be that tests could use different versions than production. That tests could themselves specify multiple versions at once, and that tests would be out-of-sync with each other, and with the production code. So I would strongly vote against that.
-Robert
From: Udo Kohlmeyer <ud...@vmware.com>
Date: Monday, October 11, 2021 at 5:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Hi there Robert,
Yes, you did work with me, but the solution to add my custom versions of jars that I want to test with, into the `DependencyConstraints` file just feels like the wrong approach. The prior art you refer to, is a specific version of tomcat we want each module to use. It is no different to specifying the exact artifact version we want for production code. My case is that I don’t want my artifact version dependencies to leak outside of the test module.
I think, the fact that the decision to elevate spotless from cosmetic formatter to actually having a significant impact, was done unilaterally. Please don’t get me wrong, I love the fact that we want to fix the problem where developers are not following good practices. But I think I am digging in here, because I believe we missed an opportunity to get more voices in this decision.
I also firmly believe that spotless is overstepping the “cosmetic” boundary. I think many developers would feel upset if their code were to be changed. If spotless suddenly replaces a for-loop with a stream implementation.
I think it is one thing to decide if gradle should be formatted with 2 spaces vs 4, or when and how lines wrap, and it’s another for spotless to just remove version information and force one consistent version even if the version was specifically overriden.
I wonder if we can maybe reduce the area of effect of this change and really only force the version formatter to activate for the ‘main’ configuration and leave every other configuration type (test, distributedTest, integrationTest, acceptanceTest, upgradeTest) alone.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Tuesday, October 12, 2021 at 10:38 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Currently there is no way to exclude certain code blocks from the malicious code formatter. Therefore, my tests that require a specific hardcoded version of Spring to be used, will always either fail the validation or tests will fail due to spotless changing my testing scenario by affecting the version of the library that we have to use.
Udo,
I have worked with you to accomplish your goal of not automatically “fixing” the inclusion of dependency versions in the build files, as your work case wants to do something special there. The formatter now throws when there is a naked version-string on the dependency line, as we discussed.
If you want to continue to work around this, there are many cases of prior art in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific versions are allowed.
Good luck.
-Robert Houghton
Re: Geode-9621: Spotless overstepping its boundary of formatter
Posted by Udo Kohlmeyer <ud...@vmware.com>.
I had never explicitly asked this, but what was the main driver for this ticket? Was there a surge in explicit version definitions in gradle files? I mean we as committers have agreed to using BOM constraints.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Wednesday, October 13, 2021 at 4:32 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Again, Spotless is not changing code. That was a bad change, as you pointed out, and we modified Spotless to throw an exception instead of stripping out the version string indiscriminately.
We surely could make Spotless only apply to the main source set. The cost would be that tests could use different versions than production. That tests could themselves specify multiple versions at once, and that tests would be out-of-sync with each other, and with the production code. So I would strongly vote against that.
-Robert
From: Udo Kohlmeyer <ud...@vmware.com>
Date: Monday, October 11, 2021 at 5:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Hi there Robert,
Yes, you did work with me, but the solution to add my custom versions of jars that I want to test with, into the `DependencyConstraints` file just feels like the wrong approach. The prior art you refer to, is a specific version of tomcat we want each module to use. It is no different to specifying the exact artifact version we want for production code. My case is that I don’t want my artifact version dependencies to leak outside of the test module.
I think, the fact that the decision to elevate spotless from cosmetic formatter to actually having a significant impact, was done unilaterally. Please don’t get me wrong, I love the fact that we want to fix the problem where developers are not following good practices. But I think I am digging in here, because I believe we missed an opportunity to get more voices in this decision.
I also firmly believe that spotless is overstepping the “cosmetic” boundary. I think many developers would feel upset if their code were to be changed. If spotless suddenly replaces a for-loop with a stream implementation.
I think it is one thing to decide if gradle should be formatted with 2 spaces vs 4, or when and how lines wrap, and it’s another for spotless to just remove version information and force one consistent version even if the version was specifically overriden.
I wonder if we can maybe reduce the area of effect of this change and really only force the version formatter to activate for the ‘main’ configuration and leave every other configuration type (test, distributedTest, integrationTest, acceptanceTest, upgradeTest) alone.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Tuesday, October 12, 2021 at 10:38 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Currently there is no way to exclude certain code blocks from the malicious code formatter. Therefore, my tests that require a specific hardcoded version of Spring to be used, will always either fail the validation or tests will fail due to spotless changing my testing scenario by affecting the version of the library that we have to use.
Udo,
I have worked with you to accomplish your goal of not automatically “fixing” the inclusion of dependency versions in the build files, as your work case wants to do something special there. The formatter now throws when there is a naked version-string on the dependency line, as we discussed.
If you want to continue to work around this, there are many cases of prior art in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific versions are allowed.
Good luck.
-Robert Houghton
Re: Geode-9621: Spotless overstepping its boundary of formatter
Posted by Robert Houghton <rh...@vmware.com>.
Again, Spotless is not changing code. That was a bad change, as you pointed out, and we modified Spotless to throw an exception instead of stripping out the version string indiscriminately.
We surely could make Spotless only apply to the main source set. The cost would be that tests could use different versions than production. That tests could themselves specify multiple versions at once, and that tests would be out-of-sync with each other, and with the production code. So I would strongly vote against that.
-Robert
From: Udo Kohlmeyer <ud...@vmware.com>
Date: Monday, October 11, 2021 at 5:32 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Hi there Robert,
Yes, you did work with me, but the solution to add my custom versions of jars that I want to test with, into the `DependencyConstraints` file just feels like the wrong approach. The prior art you refer to, is a specific version of tomcat we want each module to use. It is no different to specifying the exact artifact version we want for production code. My case is that I don’t want my artifact version dependencies to leak outside of the test module.
I think, the fact that the decision to elevate spotless from cosmetic formatter to actually having a significant impact, was done unilaterally. Please don’t get me wrong, I love the fact that we want to fix the problem where developers are not following good practices. But I think I am digging in here, because I believe we missed an opportunity to get more voices in this decision.
I also firmly believe that spotless is overstepping the “cosmetic” boundary. I think many developers would feel upset if their code were to be changed. If spotless suddenly replaces a for-loop with a stream implementation.
I think it is one thing to decide if gradle should be formatted with 2 spaces vs 4, or when and how lines wrap, and it’s another for spotless to just remove version information and force one consistent version even if the version was specifically overriden.
I wonder if we can maybe reduce the area of effect of this change and really only force the version formatter to activate for the ‘main’ configuration and leave every other configuration type (test, distributedTest, integrationTest, acceptanceTest, upgradeTest) alone.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Tuesday, October 12, 2021 at 10:38 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Currently there is no way to exclude certain code blocks from the malicious code formatter. Therefore, my tests that require a specific hardcoded version of Spring to be used, will always either fail the validation or tests will fail due to spotless changing my testing scenario by affecting the version of the library that we have to use.
Udo,
I have worked with you to accomplish your goal of not automatically “fixing” the inclusion of dependency versions in the build files, as your work case wants to do something special there. The formatter now throws when there is a naked version-string on the dependency line, as we discussed.
If you want to continue to work around this, there are many cases of prior art in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific versions are allowed.
Good luck.
-Robert Houghton
Re: Geode-9621: Spotless overstepping its boundary of formatter
Posted by Udo Kohlmeyer <ud...@vmware.com>.
Hi there Robert,
Yes, you did work with me, but the solution to add my custom versions of jars that I want to test with, into the `DependencyConstraints` file just feels like the wrong approach. The prior art you refer to, is a specific version of tomcat we want each module to use. It is no different to specifying the exact artifact version we want for production code. My case is that I don’t want my artifact version dependencies to leak outside of the test module.
I think, the fact that the decision to elevate spotless from cosmetic formatter to actually having a significant impact, was done unilaterally. Please don’t get me wrong, I love the fact that we want to fix the problem where developers are not following good practices. But I think I am digging in here, because I believe we missed an opportunity to get more voices in this decision.
I also firmly believe that spotless is overstepping the “cosmetic” boundary. I think many developers would feel upset if their code were to be changed. If spotless suddenly replaces a for-loop with a stream implementation.
I think it is one thing to decide if gradle should be formatted with 2 spaces vs 4, or when and how lines wrap, and it’s another for spotless to just remove version information and force one consistent version even if the version was specifically overriden.
I wonder if we can maybe reduce the area of effect of this change and really only force the version formatter to activate for the ‘main’ configuration and leave every other configuration type (test, distributedTest, integrationTest, acceptanceTest, upgradeTest) alone.
--Udo
From: Robert Houghton <rh...@vmware.com>
Date: Tuesday, October 12, 2021 at 10:38 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Currently there is no way to exclude certain code blocks from the malicious code formatter. Therefore, my tests that require a specific hardcoded version of Spring to be used, will always either fail the validation or tests will fail due to spotless changing my testing scenario by affecting the version of the library that we have to use.
Udo,
I have worked with you to accomplish your goal of not automatically “fixing” the inclusion of dependency versions in the build files, as your work case wants to do something special there. The formatter now throws when there is a naked version-string on the dependency line, as we discussed.
If you want to continue to work around this, there are many cases of prior art in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific versions are allowed.
Good luck.
-Robert Houghton
Re: Geode-9621: Spotless overstepping its boundary of formatter
Posted by Robert Houghton <rh...@vmware.com>.
Currently there is no way to exclude certain code blocks from the malicious code formatter. Therefore, my tests that require a specific hardcoded version of Spring to be used, will always either fail the validation or tests will fail due to spotless changing my testing scenario by affecting the version of the library that we have to use.
Udo,
I have worked with you to accomplish your goal of not automatically “fixing” the inclusion of dependency versions in the build files, as your work case wants to do something special there. The formatter now throws when there is a naked version-string on the dependency line, as we discussed.
If you want to continue to work around this, there are many cases of prior art in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific versions are allowed.
Good luck.
-Robert Houghton