o.a.w.util.license package in production source folder?
Locked
12
12
o.a.w.util.license package in production source folder?
 
|
Hi all,
Does anyone know why org.apache.wicket.util.license is in wicket-util's production source directory? I'm guessing it has something to do with the desire to get the license plugin to fire every time a build is made, but if that's the case, it would be better handled as a Maven plugin. It's not a test and it's not a part of any public API. I'm happy to create a plugin if that's the case, please let me know. Cheers, Brian |
Re: o.a.w.util.license package in production source folder?
|
|
oic, there's a ApacheLicenseHeaderTest in every project.
I'm in the process of isolating the junit.framework package to a test dependency so JUnit is not a dependency in production code. If it were made into a plugin, the instances of per-project ApacheLicenseHeader configuration would need to come from the POM. That's kind of where it belongs (it's part of the build, after all), but it could easily be made into a configuration file that resides in each project to keep the POMs clean. Failing that, creating a separate module to contain o.a.w.util.license that is a test scope dependency would be a last resort. I'm going to go ahead and create a plugin that reads a configuration file in each project. Some of the configurations are lengthy (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be a mess in the pom. On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: > Hi all, > > Does anyone know why org.apache.wicket.util.license is in wicket-util's production source directory? I'm guessing it has something to do with the desire to get the license plugin to fire every time a build is made, but if that's the case, it would be better handled as a Maven plugin. It's not a test and it's not a part of any public API. > > I'm happy to create a plugin if that's the case, please let me know. > > Cheers, Brian |
Re: o.a.w.util.license package in production source folder?
|
Hi Brian,
The main user of JUnit in production is WicketTester. About ApacheLicenceTest - Jeremy tried to replace it with com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but didn't finish it. On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]> wrote: > oic, there's a ApacheLicenseHeaderTest in every project. > > I'm in the process of isolating the junit.framework package to a test dependency so JUnit is not a dependency in production code. If it were made into a plugin, the instances of per-project ApacheLicenseHeader configuration would need to come from the POM. That's kind of where it belongs (it's part of the build, after all), but it could easily be made into a configuration file that resides in each project to keep the POMs clean. > > Failing that, creating a separate module to contain o.a.w.util.license that is a test scope dependency would be a last resort. > > I'm going to go ahead and create a plugin that reads a configuration file in each project. Some of the configurations are lengthy (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be a mess in the pom. > > > On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: > >> Hi all, >> >> Does anyone know why org.apache.wicket.util.license is in wicket-util's production source directory? I'm guessing it has something to do with the desire to get the license plugin to fire every time a build is made, but if that's the case, it would be better handled as a Maven plugin. It's not a test and it's not a part of any public API. >> >> I'm happy to create a plugin if that's the case, please let me know. >> >> Cheers, Brian > > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com |
|
The problem with com.mycila.maven-license-plugin:maven-license-plugin
as far as i remember is that is not yet published in central maven repository, so it cannot be used without adding their repo. in the pom.xml which is a problem if you are trying to get your project deployed in OSS Sonatype. On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] <[hidden email]> wrote: Hi Brian, -- JC |
Re: o.a.w.util.license package in production source folder?
|
|
Hi guys, thanks for the responses. The repository issue (as well as an unknown about outside plugins) was a concern, part of why I started a custom plugin. But if folks are comfortable with it, I think it's the right way to go. It's used in Brix and it's been very robust and convenient.
I created a branch at https://github.com/topping/wicket/tree/myclila-plugin containing the changes. There are a lot of them and it took most of the day to get it right. The plugin expects the license header to be formatted slightly differently (for instance using "/**" instead of "/*" to start a Java header). Their site suggests using <aggregation>, but that results in all the configuration being in the parent POM, something that isn't very good encapsulation of configuration. So I broke it out between projects so it's easier to maintain. As for the specific excludes, I may not have precisely the same excludes that the old test cases had. I started by copying them to the best of my perception, then tuned them for the tests (which seems to be the most sensitive aspect). Can anyone review the patch to see if there are any obvious mistakes? If not, it would be very helpful for the OSGi effort if we could get this patch applied. Removing the dependency on JUnit from wicket-util is pretty important to the effort, and I think this provides benefits to the project moving forward as well. Please let me know what I can do to facilitate. Kind regards, Brian On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: > The problem with com.mycila.maven-license-plugin:maven-license-plugin > as far as i remember is that is not yet published in central maven > repository, so it cannot be used without adding their repo. in the pom.xml > which is a problem if you are trying to get your project deployed in OSS > Sonatype. > > On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < > [hidden email]> wrote: > >> Hi Brian, >> >> The main user of JUnit in production is WicketTester. >> >> About ApacheLicenceTest - Jeremy tried to replace it with >> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >> didn't finish it. >> >> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]<http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >> wrote: >> >>> oic, there's a ApacheLicenseHeaderTest in every project. >>> >>> I'm in the process of isolating the junit.framework package to a test >> dependency so JUnit is not a dependency in production code. If it were made >> into a plugin, the instances of per-project ApacheLicenseHeader >> configuration would need to come from the POM. That's kind of where it >> belongs (it's part of the build, after all), but it could easily be made >> into a configuration file that resides in each project to keep the POMs >> clean. >>> >>> Failing that, creating a separate module to contain o.a.w.util.license >> that is a test scope dependency would be a last resort. >>> >>> I'm going to go ahead and create a plugin that reads a configuration file >> in each project. Some of the configurations are lengthy >> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be a >> mess in the pom. >>> >>> >>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >>> >>>> Hi all, >>>> >>>> Does anyone know why org.apache.wicket.util.license is in wicket-util's >> production source directory? I'm guessing it has something to do with the >> desire to get the license plugin to fire every time a build is made, but if >> that's the case, it would be better handled as a Maven plugin. It's not a >> test and it's not a part of any public API. >>>> >>>> I'm happy to create a plugin if that's the case, please let me know. >>>> >>>> Cheers, Brian >>> >>> >> >> >> >> -- >> Martin Grigorov >> jWeekend >> Training, Consulting, Development >> http://jWeekend.com >> >> >> ------------------------------ >> If you reply to this email, your message will be added to the discussion >> below: >> >> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >> To start a new topic under Apache Wicket, email >> [hidden email] >> To unsubscribe from Apache Wicket, click here< >> >> > > > > -- > > JC > > > -- > View this message in context: http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html > Sent from the Forum for Wicket Core developers mailing list archive at Nabble.com. |
|
Hey guys,
I just want to jump in here. While I think it a good idea to check license headers via a plugin instead of a junit tests this is not a "no-go" for the osgification. There are various libs out there importing org.junit... in the compile phase instead of the test-phase (although not required). At Servicemix such libs are typically wrapped using the ;optional:=true attribute. Since junit is not required at runtime I think we can go the same way for wicket here. WDYT? Kind regards, Andreas On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: > Hi guys, thanks for the responses. The repository issue (as well as an > unknown about outside plugins) was a concern, part of why I started a custom > plugin. But if folks are comfortable with it, I think it's the right way to > go. It's used in Brix and it's been very robust and convenient. > > I created a branch at > https://github.com/topping/wicket/tree/myclila-plugin containing the > changes. There are a lot of them and it took most of the day to get it > right. The plugin expects the license header to be formatted slightly > differently (for instance using "/**" instead of "/*" to start a Java > header). Their site suggests using <aggregation>, but that results in all > the configuration being in the parent POM, something that isn't very good > encapsulation of configuration. So I broke it out between projects so it's > easier to maintain. > > As for the specific excludes, I may not have precisely the same excludes > that the old test cases had. I started by copying them to the best of my > perception, then tuned them for the tests (which seems to be the most > sensitive aspect). Can anyone review the patch to see if there are any > obvious mistakes? > > If not, it would be very helpful for the OSGi effort if we could get this > patch applied. Removing the dependency on JUnit from wicket-util is pretty > important to the effort, and I think this provides benefits to the project > moving forward as well. > > Please let me know what I can do to facilitate. > > Kind regards, Brian > > On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: > > > The problem with com.mycila.maven-license-plugin:maven-license-plugin > > as far as i remember is that is not yet published in central maven > > repository, so it cannot be used without adding their repo. in the > pom.xml > > which is a problem if you are trying to get your project deployed in OSS > > Sonatype. > > > > On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < > > [hidden email]> wrote: > > > >> Hi Brian, > >> > >> The main user of JUnit in production is WicketTester. > >> > >> About ApacheLicenceTest - Jeremy tried to replace it with > >> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but > >> didn't finish it. > >> > >> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< > http://user/SendEmail.jtp?type=node&node=3742539&i=0>> > >> wrote: > >> > >>> oic, there's a ApacheLicenseHeaderTest in every project. > >>> > >>> I'm in the process of isolating the junit.framework package to a test > >> dependency so JUnit is not a dependency in production code. If it were > made > >> into a plugin, the instances of per-project ApacheLicenseHeader > >> configuration would need to come from the POM. That's kind of where it > >> belongs (it's part of the build, after all), but it could easily be made > >> into a configuration file that resides in each project to keep the POMs > >> clean. > >>> > >>> Failing that, creating a separate module to contain o.a.w.util.license > >> that is a test scope dependency would be a last resort. > >>> > >>> I'm going to go ahead and create a plugin that reads a configuration > file > >> in each project. Some of the configurations are lengthy > >> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be > a > >> mess in the pom. > >>> > >>> > >>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: > >>> > >>>> Hi all, > >>>> > >>>> Does anyone know why org.apache.wicket.util.license is in > wicket-util's > >> production source directory? I'm guessing it has something to do with > the > >> desire to get the license plugin to fire every time a build is made, but > if > >> that's the case, it would be better handled as a Maven plugin. It's not > a > >> test and it's not a part of any public API. > >>>> > >>>> I'm happy to create a plugin if that's the case, please let me know. > >>>> > >>>> Cheers, Brian > >>> > >>> > >> > >> > >> > >> -- > >> Martin Grigorov > >> jWeekend > >> Training, Consulting, Development > >> http://jWeekend.com > >> > >> > >> ------------------------------ > >> If you reply to this email, your message will be added to the discussion > >> below: > >> > >> > http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html > >> To start a new topic under Apache Wicket, email > >> [hidden email] > >> To unsubscribe from Apache Wicket, click here< > >. > >> > >> > > > > > > > > -- > > > > JC > > > > > > -- > > View this message in context: > http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html > > Sent from the Forum for Wicket Core developers mailing list archive at > Nabble.com. > > |
Re: o.a.w.util.license package in production source folder?
|
|
Hi,
The change at https://github.com/topping/wicket/commit/8f08c2082f90fcd54a7e4734b892d13e610a47bf looks OK to me but I'm not quite familiar with the unit test based solution (the current one). I'm not sure whether there are some (unknown for me) rules like specific formatting which may become broken now. I guess Andreas' solution will be needed at least for wicket-core because as I said (Base)WicketTester depends on JUnit - both compile and runtime. On Mon, Aug 15, 2011 at 9:44 AM, Andreas Pieber <[hidden email]> wrote: > Hey guys, > > I just want to jump in here. While I think it a good idea to check license > headers via a plugin instead of a junit tests this is not a "no-go" for the > osgification. There are various libs out there importing org.junit... in the > compile phase instead of the test-phase (although not required). At > Servicemix such libs are typically wrapped using the ;optional:=true > attribute. Since junit is not required at runtime I think we can go the same > way for wicket here. > > WDYT? > > Kind regards, > Andreas > > On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: > >> Hi guys, thanks for the responses. The repository issue (as well as an >> unknown about outside plugins) was a concern, part of why I started a custom >> plugin. But if folks are comfortable with it, I think it's the right way to >> go. It's used in Brix and it's been very robust and convenient. >> >> I created a branch at >> https://github.com/topping/wicket/tree/myclila-plugin containing the >> changes. There are a lot of them and it took most of the day to get it >> right. The plugin expects the license header to be formatted slightly >> differently (for instance using "/**" instead of "/*" to start a Java >> header). Their site suggests using <aggregation>, but that results in all >> the configuration being in the parent POM, something that isn't very good >> encapsulation of configuration. So I broke it out between projects so it's >> easier to maintain. >> >> As for the specific excludes, I may not have precisely the same excludes >> that the old test cases had. I started by copying them to the best of my >> perception, then tuned them for the tests (which seems to be the most >> sensitive aspect). Can anyone review the patch to see if there are any >> obvious mistakes? >> >> If not, it would be very helpful for the OSGi effort if we could get this >> patch applied. Removing the dependency on JUnit from wicket-util is pretty >> important to the effort, and I think this provides benefits to the project >> moving forward as well. >> >> Please let me know what I can do to facilitate. >> >> Kind regards, Brian >> >> On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: >> >> > The problem with com.mycila.maven-license-plugin:maven-license-plugin >> > as far as i remember is that is not yet published in central maven >> > repository, so it cannot be used without adding their repo. in the >> pom.xml >> > which is a problem if you are trying to get your project deployed in OSS >> > Sonatype. >> > >> > On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < >> > [hidden email]> wrote: >> > >> >> Hi Brian, >> >> >> >> The main user of JUnit in production is WicketTester. >> >> >> >> About ApacheLicenceTest - Jeremy tried to replace it with >> >> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >> >> didn't finish it. >> >> >> >> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< >> http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >> >> wrote: >> >> >> >>> oic, there's a ApacheLicenseHeaderTest in every project. >> >>> >> >>> I'm in the process of isolating the junit.framework package to a test >> >> dependency so JUnit is not a dependency in production code. If it were >> made >> >> into a plugin, the instances of per-project ApacheLicenseHeader >> >> configuration would need to come from the POM. That's kind of where it >> >> belongs (it's part of the build, after all), but it could easily be made >> >> into a configuration file that resides in each project to keep the POMs >> >> clean. >> >>> >> >>> Failing that, creating a separate module to contain o.a.w.util.license >> >> that is a test scope dependency would be a last resort. >> >>> >> >>> I'm going to go ahead and create a plugin that reads a configuration >> file >> >> in each project. Some of the configurations are lengthy >> >> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be >> a >> >> mess in the pom. >> >>> >> >>> >> >>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >> >>> >> >>>> Hi all, >> >>>> >> >>>> Does anyone know why org.apache.wicket.util.license is in >> wicket-util's >> >> production source directory? I'm guessing it has something to do with >> the >> >> desire to get the license plugin to fire every time a build is made, but >> if >> >> that's the case, it would be better handled as a Maven plugin. It's not >> a >> >> test and it's not a part of any public API. >> >>>> >> >>>> I'm happy to create a plugin if that's the case, please let me know. >> >>>> >> >>>> Cheers, Brian >> >>> >> >>> >> >> >> >> >> >> >> >> -- >> >> Martin Grigorov >> >> jWeekend >> >> Training, Consulting, Development >> >> http://jWeekend.com >> >> >> >> >> >> ------------------------------ >> >> If you reply to this email, your message will be added to the discussion >> >> below: >> >> >> >> >> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >> >> To start a new topic under Apache Wicket, email >> >> [hidden email] >> >> To unsubscribe from Apache Wicket, click here< >> >. >> >> >> >> >> > >> > >> > >> > -- >> > >> > JC >> > >> > >> > -- >> > View this message in context: >> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html >> > Sent from the Forum for Wicket Core developers mailing list archive at >> Nabble.com. >> >> > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com |
Re: o.a.w.util.license package in production source folder?
|
|
Is it possible to configure the plugin to fail on unknown files ?
E.g. currently it excludes 'target/' folder and checks files with extensions .java, .xml, .html, .properties, .js, .css and .vm. Is it possible to add some global excludes for images and configure it to fail the verification if an unknown file is found. I.e. if there is src/main/java/org/apache/wicket/SomeFile.blah and it is not explicitly excluded then fail Additionally can you add some comments around why some resources are excluded ? I just saw wicket-core/pom.xml has: <plugin> 83 + <groupId>com.mycila.maven-license-plugin</groupId> 84 + <artifactId>maven-license-plugin</artifactId> 85 + <configuration> 86 + <header>${project.parent.basedir}/licenses/asf-license-header.txt</header> 87 + <excludes> 88 + <exclude>src/main/java/org/apache/wicket/**/*.properties</exclude> and I wonder why line 88 is there. I know current JUnit based tests also have some exclusions but I'm not sure whether they have explanation "why". On Mon, Aug 15, 2011 at 10:06 AM, Martin Grigorov <[hidden email]> wrote: > Hi, > > The change at https://github.com/topping/wicket/commit/8f08c2082f90fcd54a7e4734b892d13e610a47bf > looks OK to me but I'm not quite familiar with the unit test based > solution (the current one). I'm not sure whether there are some > (unknown for me) rules like specific formatting which may become > broken now. > > I guess Andreas' solution will be needed at least for wicket-core > because as I said (Base)WicketTester depends on JUnit - both compile > and runtime. > > On Mon, Aug 15, 2011 at 9:44 AM, Andreas Pieber <[hidden email]> wrote: >> Hey guys, >> >> I just want to jump in here. While I think it a good idea to check license >> headers via a plugin instead of a junit tests this is not a "no-go" for the >> osgification. There are various libs out there importing org.junit... in the >> compile phase instead of the test-phase (although not required). At >> Servicemix such libs are typically wrapped using the ;optional:=true >> attribute. Since junit is not required at runtime I think we can go the same >> way for wicket here. >> >> WDYT? >> >> Kind regards, >> Andreas >> >> On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: >> >>> Hi guys, thanks for the responses. The repository issue (as well as an >>> unknown about outside plugins) was a concern, part of why I started a custom >>> plugin. But if folks are comfortable with it, I think it's the right way to >>> go. It's used in Brix and it's been very robust and convenient. >>> >>> I created a branch at >>> https://github.com/topping/wicket/tree/myclila-plugin containing the >>> changes. There are a lot of them and it took most of the day to get it >>> right. The plugin expects the license header to be formatted slightly >>> differently (for instance using "/**" instead of "/*" to start a Java >>> header). Their site suggests using <aggregation>, but that results in all >>> the configuration being in the parent POM, something that isn't very good >>> encapsulation of configuration. So I broke it out between projects so it's >>> easier to maintain. >>> >>> As for the specific excludes, I may not have precisely the same excludes >>> that the old test cases had. I started by copying them to the best of my >>> perception, then tuned them for the tests (which seems to be the most >>> sensitive aspect). Can anyone review the patch to see if there are any >>> obvious mistakes? >>> >>> If not, it would be very helpful for the OSGi effort if we could get this >>> patch applied. Removing the dependency on JUnit from wicket-util is pretty >>> important to the effort, and I think this provides benefits to the project >>> moving forward as well. >>> >>> Please let me know what I can do to facilitate. >>> >>> Kind regards, Brian >>> >>> On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: >>> >>> > The problem with com.mycila.maven-license-plugin:maven-license-plugin >>> > as far as i remember is that is not yet published in central maven >>> > repository, so it cannot be used without adding their repo. in the >>> pom.xml >>> > which is a problem if you are trying to get your project deployed in OSS >>> > Sonatype. >>> > >>> > On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < >>> > [hidden email]> wrote: >>> > >>> >> Hi Brian, >>> >> >>> >> The main user of JUnit in production is WicketTester. >>> >> >>> >> About ApacheLicenceTest - Jeremy tried to replace it with >>> >> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >>> >> didn't finish it. >>> >> >>> >> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< >>> http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >>> >> wrote: >>> >> >>> >>> oic, there's a ApacheLicenseHeaderTest in every project. >>> >>> >>> >>> I'm in the process of isolating the junit.framework package to a test >>> >> dependency so JUnit is not a dependency in production code. If it were >>> made >>> >> into a plugin, the instances of per-project ApacheLicenseHeader >>> >> configuration would need to come from the POM. That's kind of where it >>> >> belongs (it's part of the build, after all), but it could easily be made >>> >> into a configuration file that resides in each project to keep the POMs >>> >> clean. >>> >>> >>> >>> Failing that, creating a separate module to contain o.a.w.util.license >>> >> that is a test scope dependency would be a last resort. >>> >>> >>> >>> I'm going to go ahead and create a plugin that reads a configuration >>> file >>> >> in each project. Some of the configurations are lengthy >>> >> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be >>> a >>> >> mess in the pom. >>> >>> >>> >>> >>> >>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >>> >>> >>> >>>> Hi all, >>> >>>> >>> >>>> Does anyone know why org.apache.wicket.util.license is in >>> wicket-util's >>> >> production source directory? I'm guessing it has something to do with >>> the >>> >> desire to get the license plugin to fire every time a build is made, but >>> if >>> >> that's the case, it would be better handled as a Maven plugin. It's not >>> a >>> >> test and it's not a part of any public API. >>> >>>> >>> >>>> I'm happy to create a plugin if that's the case, please let me know. >>> >>>> >>> >>>> Cheers, Brian >>> >>> >>> >>> >>> >> >>> >> >>> >> >>> >> -- >>> >> Martin Grigorov >>> >> jWeekend >>> >> Training, Consulting, Development >>> >> http://jWeekend.com >>> >> >>> >> >>> >> ------------------------------ >>> >> If you reply to this email, your message will be added to the discussion >>> >> below: >>> >> >>> >> >>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >>> >> To start a new topic under Apache Wicket, email >>> >> [hidden email] >>> >> To unsubscribe from Apache Wicket, click here< >>> >. >>> >> >>> >> >>> > >>> > >>> > >>> > -- >>> > >>> > JC >>> > >>> > >>> > -- >>> > View this message in context: >>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html >>> > Sent from the Forum for Wicket Core developers mailing list archive at >>> Nabble.com. >>> >>> >> > > > > -- > Martin Grigorov > jWeekend > Training, Consulting, Development > http://jWeekend.com > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com |
OT: Code quality (was Re: o.a.w.util.license package in production source folder?)
|
|
In reply to this post by pieber
Hi guys,
Just to provide some personal perspective and it's somewhat off-topic... there's always a lot of things we can work around here (code can always be compensated for with more code), but I think there is a responsibility with all code to "leave it cleaner than you found it". To say it differently, to me, any amount of effort today to keep things clean is worth it, because tomorrow (with additional code thrown on), it may take twice as long to undo it and we may not have options to work around the problem any longer (thus forcing that we cannot avoid cleaning it up with twice the investment). When I looked at the actual usages of JUnit, it was primarily on junit.framework.Assert in about three or four random files, when in fact the standing pattern is to use o.a.w.util.lang.Args or throw an IllegalStateException if there is a problem with incomplete initialization. In this case, removing JUnit as a dependency from util in fact improves the code, and in the process does not bury a dependency even deeper. In fact, there was a comment in one of the POMs alluding to the question of why JUnit was a runtime dependency. I don't think I am alone in believing that it should have been removed. This doesn't answer to o.a.w.util.tester.WicketTester, but that's better answered in Martin's email. Cheers and thanks, Brian On Aug 15, 2011, at 2:44 AM, Andreas Pieber wrote: > Hey guys, > > I just want to jump in here. While I think it a good idea to check license > headers via a plugin instead of a junit tests this is not a "no-go" for the > osgification. There are various libs out there importing org.junit... in the > compile phase instead of the test-phase (although not required). At > Servicemix such libs are typically wrapped using the ;optional:=true > attribute. Since junit is not required at runtime I think we can go the same > way for wicket here. > > WDYT? > > Kind regards, > Andreas > > On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: > >> Hi guys, thanks for the responses. The repository issue (as well as an >> unknown about outside plugins) was a concern, part of why I started a custom >> plugin. But if folks are comfortable with it, I think it's the right way to >> go. It's used in Brix and it's been very robust and convenient. >> >> I created a branch at >> https://github.com/topping/wicket/tree/myclila-plugin containing the >> changes. There are a lot of them and it took most of the day to get it >> right. The plugin expects the license header to be formatted slightly >> differently (for instance using "/**" instead of "/*" to start a Java >> header). Their site suggests using <aggregation>, but that results in all >> the configuration being in the parent POM, something that isn't very good >> encapsulation of configuration. So I broke it out between projects so it's >> easier to maintain. >> >> As for the specific excludes, I may not have precisely the same excludes >> that the old test cases had. I started by copying them to the best of my >> perception, then tuned them for the tests (which seems to be the most >> sensitive aspect). Can anyone review the patch to see if there are any >> obvious mistakes? >> >> If not, it would be very helpful for the OSGi effort if we could get this >> patch applied. Removing the dependency on JUnit from wicket-util is pretty >> important to the effort, and I think this provides benefits to the project >> moving forward as well. >> >> Please let me know what I can do to facilitate. >> >> Kind regards, Brian >> >> On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: >> >>> The problem with com.mycila.maven-license-plugin:maven-license-plugin >>> as far as i remember is that is not yet published in central maven >>> repository, so it cannot be used without adding their repo. in the >> pom.xml >>> which is a problem if you are trying to get your project deployed in OSS >>> Sonatype. >>> >>> On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < >>> [hidden email]> wrote: >>> >>>> Hi Brian, >>>> >>>> The main user of JUnit in production is WicketTester. >>>> >>>> About ApacheLicenceTest - Jeremy tried to replace it with >>>> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >>>> didn't finish it. >>>> >>>> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< >> http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >>>> wrote: >>>> >>>>> oic, there's a ApacheLicenseHeaderTest in every project. >>>>> >>>>> I'm in the process of isolating the junit.framework package to a test >>>> dependency so JUnit is not a dependency in production code. If it were >> made >>>> into a plugin, the instances of per-project ApacheLicenseHeader >>>> configuration would need to come from the POM. That's kind of where it >>>> belongs (it's part of the build, after all), but it could easily be made >>>> into a configuration file that resides in each project to keep the POMs >>>> clean. >>>>> >>>>> Failing that, creating a separate module to contain o.a.w.util.license >>>> that is a test scope dependency would be a last resort. >>>>> >>>>> I'm going to go ahead and create a plugin that reads a configuration >> file >>>> in each project. Some of the configurations are lengthy >>>> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be >> a >>>> mess in the pom. >>>>> >>>>> >>>>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >>>>> >>>>>> Hi all, >>>>>> >>>>>> Does anyone know why org.apache.wicket.util.license is in >> wicket-util's >>>> production source directory? I'm guessing it has something to do with >> the >>>> desire to get the license plugin to fire every time a build is made, but >> if >>>> that's the case, it would be better handled as a Maven plugin. It's not >> a >>>> test and it's not a part of any public API. >>>>>> >>>>>> I'm happy to create a plugin if that's the case, please let me know. >>>>>> >>>>>> Cheers, Brian >>>>> >>>>> >>>> >>>> >>>> >>>> -- >>>> Martin Grigorov >>>> jWeekend >>>> Training, Consulting, Development >>>> http://jWeekend.com >>>> >>>> >>>> ------------------------------ >>>> If you reply to this email, your message will be added to the discussion >>>> below: >>>> >>>> >> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >>>> To start a new topic under Apache Wicket, email >>>> [hidden email] >>>> To unsubscribe from Apache Wicket, click here< >> . >>>> >>>> >>> >>> >>> >>> -- >>> >>> JC >>> >>> >>> -- >>> View this message in context: >> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html >>> Sent from the Forum for Wicket Core developers mailing list archive at >> Nabble.com. >> >> |
Re: o.a.w.util.license package in production source folder?
|
|
In reply to this post by Martin Grigorov-4
Hi Martin, comments inline.
On Aug 15, 2011, at 3:50 AM, Martin Grigorov wrote: > Is it possible to configure the plugin to fail on unknown files ? There are no limitations of what we can do, even if we need to fork the mycila plugin to do it. :-) But if you mean "can the plugin as written deal with unknown files, the only way to do that would be to include everything (<include>**</include>) and then write an exclude that matches every file that should not be included. This will catch new files that are unknown in the way you mentioned. I didn't come to believe that the old system did this though, just that it handled the six or so types (java, js, xml, vm, css, and a couple others I don't remember ATM). > E.g. currently it excludes 'target/' folder and checks files with > extensions .java, .xml, .html, .properties, .js, .css and .vm. Is it > possible to add some global excludes for images and configure it to > fail the verification if an unknown file is found. I.e. if there is > src/main/java/org/apache/wicket/SomeFile.blah and it is not explicitly > excluded then fail My experience with the old method of using unit tests to test that the source code had headers was not an exact process either. I had seen many files that did not have headers for no apparent reason. What I tried to do was find a "best fit" to the existing rules. With as many files as there are in the system, it would require an automated test rig to ensure that every file was being managed exactly like the old system did, and given that the old system had holes caught on visual inspection, it didn't seem that was time well-spent. > > Additionally can you add some comments around why some resources are excluded ? > I just saw wicket-core/pom.xml has: > <plugin> > 83 > + <groupId>com.mycila.maven-license-plugin</groupId> > 84 > + <artifactId>maven-license-plugin</artifactId> > 85 > + <configuration> > 86 > + <header>${project.parent.basedir}/licenses/asf-license-header.txt</header> > 87 > + <excludes> > 88 > + <exclude>src/main/java/org/apache/wicket/**/*.properties</exclude> > > and I wonder why line 88 is there. Yes, line 88 is probably incorrect. I'll take a look at it. > I know current JUnit based tests also have some exclusions but I'm not > sure whether they have explanation "why". The issue is with test data. For instance, a .html file in the test tree may not actually be used in the compilation of a test, it may be just for comparison that WicketTester created the final output that is correct. Adding a header to a file like that would cause the comparison (and the test) to fail. We could add that header to both the test input and test output, but I was trying to take the path of least impact. > > On Mon, Aug 15, 2011 at 10:06 AM, Martin Grigorov <[hidden email]> wrote: >> Hi, >> >> The change at https://github.com/topping/wicket/commit/8f08c2082f90fcd54a7e4734b892d13e610a47bf >> looks OK to me but I'm not quite familiar with the unit test based >> solution (the current one). I'm not sure whether there are some >> (unknown for me) rules like specific formatting which may become >> broken now. This doesn't seem to be the case. I'm committed to supporting the change that I made here though, so whatever minor changes are necessary, I'm happy to have issues assigned to me. >> >> I guess Andreas' solution will be needed at least for wicket-core >> because as I said (Base)WicketTester depends on JUnit - both compile >> and runtime. Yes, the best practice in this case is to have a test module containing test code that has a runtime dependency on a library such as JUnit. There was a huge amount of changes to get this in, and I overlooked that o.a.w.util.tester is in core. Having it remain that way would cause the entire body of work to be pretty, but not very useful to it's original end. Are there practical reasons that o.a.w.util.tester is in core? <snip/> Cheers, Brian |
Re: OT: Code quality (was Re: o.a.w.util.license package in production source folder?)
|
In reply to this post by Brian Topping
can we have a patch that changes those places to use Args.*? :)
-igor On Mon, Aug 15, 2011 at 7:50 AM, Brian Topping <[hidden email]> wrote: > Hi guys, > > Just to provide some personal perspective and it's somewhat off-topic... there's always a lot of things we can work around here (code can always be compensated for with more code), but I think there is a responsibility with all code to "leave it cleaner than you found it". To say it differently, to me, any amount of effort today to keep things clean is worth it, because tomorrow (with additional code thrown on), it may take twice as long to undo it and we may not have options to work around the problem any longer (thus forcing that we cannot avoid cleaning it up with twice the investment). > > When I looked at the actual usages of JUnit, it was primarily on junit.framework.Assert in about three or four random files, when in fact the standing pattern is to use o.a.w.util.lang.Args or throw an IllegalStateException if there is a problem with incomplete initialization. > > In this case, removing JUnit as a dependency from util in fact improves the code, and in the process does not bury a dependency even deeper. In fact, there was a comment in one of the POMs alluding to the question of why JUnit was a runtime dependency. I don't think I am alone in believing that it should have been removed. This doesn't answer to o.a.w.util.tester.WicketTester, but that's better answered in Martin's email. > > Cheers and thanks, > > Brian > > On Aug 15, 2011, at 2:44 AM, Andreas Pieber wrote: > >> Hey guys, >> >> I just want to jump in here. While I think it a good idea to check license >> headers via a plugin instead of a junit tests this is not a "no-go" for the >> osgification. There are various libs out there importing org.junit... in the >> compile phase instead of the test-phase (although not required). At >> Servicemix such libs are typically wrapped using the ;optional:=true >> attribute. Since junit is not required at runtime I think we can go the same >> way for wicket here. >> >> WDYT? >> >> Kind regards, >> Andreas >> >> On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: >> >>> Hi guys, thanks for the responses. The repository issue (as well as an >>> unknown about outside plugins) was a concern, part of why I started a custom >>> plugin. But if folks are comfortable with it, I think it's the right way to >>> go. It's used in Brix and it's been very robust and convenient. >>> >>> I created a branch at >>> https://github.com/topping/wicket/tree/myclila-plugin containing the >>> changes. There are a lot of them and it took most of the day to get it >>> right. The plugin expects the license header to be formatted slightly >>> differently (for instance using "/**" instead of "/*" to start a Java >>> header). Their site suggests using <aggregation>, but that results in all >>> the configuration being in the parent POM, something that isn't very good >>> encapsulation of configuration. So I broke it out between projects so it's >>> easier to maintain. >>> >>> As for the specific excludes, I may not have precisely the same excludes >>> that the old test cases had. I started by copying them to the best of my >>> perception, then tuned them for the tests (which seems to be the most >>> sensitive aspect). Can anyone review the patch to see if there are any >>> obvious mistakes? >>> >>> If not, it would be very helpful for the OSGi effort if we could get this >>> patch applied. Removing the dependency on JUnit from wicket-util is pretty >>> important to the effort, and I think this provides benefits to the project >>> moving forward as well. >>> >>> Please let me know what I can do to facilitate. >>> >>> Kind regards, Brian >>> >>> On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: >>> >>>> The problem with com.mycila.maven-license-plugin:maven-license-plugin >>>> as far as i remember is that is not yet published in central maven >>>> repository, so it cannot be used without adding their repo. in the >>> pom.xml >>>> which is a problem if you are trying to get your project deployed in OSS >>>> Sonatype. >>>> >>>> On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < >>>> [hidden email]> wrote: >>>> >>>>> Hi Brian, >>>>> >>>>> The main user of JUnit in production is WicketTester. >>>>> >>>>> About ApacheLicenceTest - Jeremy tried to replace it with >>>>> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >>>>> didn't finish it. >>>>> >>>>> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< >>> http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >>>>> wrote: >>>>> >>>>>> oic, there's a ApacheLicenseHeaderTest in every project. >>>>>> >>>>>> I'm in the process of isolating the junit.framework package to a test >>>>> dependency so JUnit is not a dependency in production code. If it were >>> made >>>>> into a plugin, the instances of per-project ApacheLicenseHeader >>>>> configuration would need to come from the POM. That's kind of where it >>>>> belongs (it's part of the build, after all), but it could easily be made >>>>> into a configuration file that resides in each project to keep the POMs >>>>> clean. >>>>>> >>>>>> Failing that, creating a separate module to contain o.a.w.util.license >>>>> that is a test scope dependency would be a last resort. >>>>>> >>>>>> I'm going to go ahead and create a plugin that reads a configuration >>> file >>>>> in each project. Some of the configurations are lengthy >>>>> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be >>> a >>>>> mess in the pom. >>>>>> >>>>>> >>>>>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >>>>>> >>>>>>> Hi all, >>>>>>> >>>>>>> Does anyone know why org.apache.wicket.util.license is in >>> wicket-util's >>>>> production source directory? I'm guessing it has something to do with >>> the >>>>> desire to get the license plugin to fire every time a build is made, but >>> if >>>>> that's the case, it would be better handled as a Maven plugin. It's not >>> a >>>>> test and it's not a part of any public API. >>>>>>> >>>>>>> I'm happy to create a plugin if that's the case, please let me know. >>>>>>> >>>>>>> Cheers, Brian >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Martin Grigorov >>>>> jWeekend >>>>> Training, Consulting, Development >>>>> http://jWeekend.com >>>>> >>>>> >>>>> ------------------------------ >>>>> If you reply to this email, your message will be added to the discussion >>>>> below: >>>>> >>>>> >>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >>>>> To start a new topic under Apache Wicket, email >>>>> [hidden email] >>>>> To unsubscribe from Apache Wicket, click here< >>> . >>>>> >>>>> >>>> >>>> >>>> >>>> -- >>>> >>>> JC >>>> >>>> >>>> -- >>>> View this message in context: >>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html >>>> Sent from the Forum for Wicket Core developers mailing list archive at >>> Nabble.com. >>> >>> > > |
Re: OT: Code quality (was Re: o.a.w.util.license package in production source folder?)
|
|
I don't know git that well! :-)
On Aug 15, 2011, at 11:46 AM, Igor Vaynberg wrote: > can we have a patch that changes those places to use Args.*? :) > > -igor > > On Mon, Aug 15, 2011 at 7:50 AM, Brian Topping <[hidden email]> wrote: >> Hi guys, >> >> Just to provide some personal perspective and it's somewhat off-topic... there's always a lot of things we can work around here (code can always be compensated for with more code), but I think there is a responsibility with all code to "leave it cleaner than you found it". To say it differently, to me, any amount of effort today to keep things clean is worth it, because tomorrow (with additional code thrown on), it may take twice as long to undo it and we may not have options to work around the problem any longer (thus forcing that we cannot avoid cleaning it up with twice the investment). >> >> When I looked at the actual usages of JUnit, it was primarily on junit.framework.Assert in about three or four random files, when in fact the standing pattern is to use o.a.w.util.lang.Args or throw an IllegalStateException if there is a problem with incomplete initialization. >> >> In this case, removing JUnit as a dependency from util in fact improves the code, and in the process does not bury a dependency even deeper. In fact, there was a comment in one of the POMs alluding to the question of why JUnit was a runtime dependency. I don't think I am alone in believing that it should have been removed. This doesn't answer to o.a.w.util.tester.WicketTester, but that's better answered in Martin's email. >> >> Cheers and thanks, >> >> Brian >> >> On Aug 15, 2011, at 2:44 AM, Andreas Pieber wrote: >> >>> Hey guys, >>> >>> I just want to jump in here. While I think it a good idea to check license >>> headers via a plugin instead of a junit tests this is not a "no-go" for the >>> osgification. There are various libs out there importing org.junit... in the >>> compile phase instead of the test-phase (although not required). At >>> Servicemix such libs are typically wrapped using the ;optional:=true >>> attribute. Since junit is not required at runtime I think we can go the same >>> way for wicket here. >>> >>> WDYT? >>> >>> Kind regards, >>> Andreas >>> >>> On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: >>> >>>> Hi guys, thanks for the responses. The repository issue (as well as an >>>> unknown about outside plugins) was a concern, part of why I started a custom >>>> plugin. But if folks are comfortable with it, I think it's the right way to >>>> go. It's used in Brix and it's been very robust and convenient. >>>> >>>> I created a branch at >>>> https://github.com/topping/wicket/tree/myclila-plugin containing the >>>> changes. There are a lot of them and it took most of the day to get it >>>> right. The plugin expects the license header to be formatted slightly >>>> differently (for instance using "/**" instead of "/*" to start a Java >>>> header). Their site suggests using <aggregation>, but that results in all >>>> the configuration being in the parent POM, something that isn't very good >>>> encapsulation of configuration. So I broke it out between projects so it's >>>> easier to maintain. >>>> >>>> As for the specific excludes, I may not have precisely the same excludes >>>> that the old test cases had. I started by copying them to the best of my >>>> perception, then tuned them for the tests (which seems to be the most >>>> sensitive aspect). Can anyone review the patch to see if there are any >>>> obvious mistakes? >>>> >>>> If not, it would be very helpful for the OSGi effort if we could get this >>>> patch applied. Removing the dependency on JUnit from wicket-util is pretty >>>> important to the effort, and I think this provides benefits to the project >>>> moving forward as well. >>>> >>>> Please let me know what I can do to facilitate. >>>> >>>> Kind regards, Brian >>>> >>>> On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: >>>> >>>>> The problem with com.mycila.maven-license-plugin:maven-license-plugin >>>>> as far as i remember is that is not yet published in central maven >>>>> repository, so it cannot be used without adding their repo. in the >>>> pom.xml >>>>> which is a problem if you are trying to get your project deployed in OSS >>>>> Sonatype. >>>>> >>>>> On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < >>>>> [hidden email]> wrote: >>>>> >>>>>> Hi Brian, >>>>>> >>>>>> The main user of JUnit in production is WicketTester. >>>>>> >>>>>> About ApacheLicenceTest - Jeremy tried to replace it with >>>>>> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >>>>>> didn't finish it. >>>>>> >>>>>> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< >>>> http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >>>>>> wrote: >>>>>> >>>>>>> oic, there's a ApacheLicenseHeaderTest in every project. >>>>>>> >>>>>>> I'm in the process of isolating the junit.framework package to a test >>>>>> dependency so JUnit is not a dependency in production code. If it were >>>> made >>>>>> into a plugin, the instances of per-project ApacheLicenseHeader >>>>>> configuration would need to come from the POM. That's kind of where it >>>>>> belongs (it's part of the build, after all), but it could easily be made >>>>>> into a configuration file that resides in each project to keep the POMs >>>>>> clean. >>>>>>> >>>>>>> Failing that, creating a separate module to contain o.a.w.util.license >>>>>> that is a test scope dependency would be a last resort. >>>>>>> >>>>>>> I'm going to go ahead and create a plugin that reads a configuration >>>> file >>>>>> in each project. Some of the configurations are lengthy >>>>>> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be >>>> a >>>>>> mess in the pom. >>>>>>> >>>>>>> >>>>>>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >>>>>>> >>>>>>>> Hi all, >>>>>>>> >>>>>>>> Does anyone know why org.apache.wicket.util.license is in >>>> wicket-util's >>>>>> production source directory? I'm guessing it has something to do with >>>> the >>>>>> desire to get the license plugin to fire every time a build is made, but >>>> if >>>>>> that's the case, it would be better handled as a Maven plugin. It's not >>>> a >>>>>> test and it's not a part of any public API. >>>>>>>> >>>>>>>> I'm happy to create a plugin if that's the case, please let me know. >>>>>>>> >>>>>>>> Cheers, Brian >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Martin Grigorov >>>>>> jWeekend >>>>>> Training, Consulting, Development >>>>>> http://jWeekend.com >>>>>> >>>>>> >>>>>> ------------------------------ >>>>>> If you reply to this email, your message will be added to the discussion >>>>>> below: >>>>>> >>>>>> >>>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >>>>>> To start a new topic under Apache Wicket, email >>>>>> [hidden email] >>>>>> To unsubscribe from Apache Wicket, click here< >>>> . >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> >>>>> JC >>>>> >>>>> >>>>> -- >>>>> View this message in context: >>>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html >>>>> Sent from the Forum for Wicket Core developers mailing list archive at >>>> Nabble.com. >>>> >>>> >> >> > |
Re: OT: Code quality (was Re: o.a.w.util.license package in production source folder?)
|
|
On Mon, Aug 15, 2011 at 6:50 PM, Brian Topping <[hidden email]> wrote:
> I don't know git that well! :-) git diff myBranch..master > some.patch > > On Aug 15, 2011, at 11:46 AM, Igor Vaynberg wrote: > >> can we have a patch that changes those places to use Args.*? :) >> >> -igor >> >> On Mon, Aug 15, 2011 at 7:50 AM, Brian Topping <[hidden email]> wrote: >>> Hi guys, >>> >>> Just to provide some personal perspective and it's somewhat off-topic... there's always a lot of things we can work around here (code can always be compensated for with more code), but I think there is a responsibility with all code to "leave it cleaner than you found it". To say it differently, to me, any amount of effort today to keep things clean is worth it, because tomorrow (with additional code thrown on), it may take twice as long to undo it and we may not have options to work around the problem any longer (thus forcing that we cannot avoid cleaning it up with twice the investment). >>> >>> When I looked at the actual usages of JUnit, it was primarily on junit.framework.Assert in about three or four random files, when in fact the standing pattern is to use o.a.w.util.lang.Args or throw an IllegalStateException if there is a problem with incomplete initialization. >>> >>> In this case, removing JUnit as a dependency from util in fact improves the code, and in the process does not bury a dependency even deeper. In fact, there was a comment in one of the POMs alluding to the question of why JUnit was a runtime dependency. I don't think I am alone in believing that it should have been removed. This doesn't answer to o.a.w.util.tester.WicketTester, but that's better answered in Martin's email. >>> >>> Cheers and thanks, >>> >>> Brian >>> >>> On Aug 15, 2011, at 2:44 AM, Andreas Pieber wrote: >>> >>>> Hey guys, >>>> >>>> I just want to jump in here. While I think it a good idea to check license >>>> headers via a plugin instead of a junit tests this is not a "no-go" for the >>>> osgification. There are various libs out there importing org.junit... in the >>>> compile phase instead of the test-phase (although not required). At >>>> Servicemix such libs are typically wrapped using the ;optional:=true >>>> attribute. Since junit is not required at runtime I think we can go the same >>>> way for wicket here. >>>> >>>> WDYT? >>>> >>>> Kind regards, >>>> Andreas >>>> >>>> On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: >>>> >>>>> Hi guys, thanks for the responses. The repository issue (as well as an >>>>> unknown about outside plugins) was a concern, part of why I started a custom >>>>> plugin. But if folks are comfortable with it, I think it's the right way to >>>>> go. It's used in Brix and it's been very robust and convenient. >>>>> >>>>> I created a branch at >>>>> https://github.com/topping/wicket/tree/myclila-plugin containing the >>>>> changes. There are a lot of them and it took most of the day to get it >>>>> right. The plugin expects the license header to be formatted slightly >>>>> differently (for instance using "/**" instead of "/*" to start a Java >>>>> header). Their site suggests using <aggregation>, but that results in all >>>>> the configuration being in the parent POM, something that isn't very good >>>>> encapsulation of configuration. So I broke it out between projects so it's >>>>> easier to maintain. >>>>> >>>>> As for the specific excludes, I may not have precisely the same excludes >>>>> that the old test cases had. I started by copying them to the best of my >>>>> perception, then tuned them for the tests (which seems to be the most >>>>> sensitive aspect). Can anyone review the patch to see if there are any >>>>> obvious mistakes? >>>>> >>>>> If not, it would be very helpful for the OSGi effort if we could get this >>>>> patch applied. Removing the dependency on JUnit from wicket-util is pretty >>>>> important to the effort, and I think this provides benefits to the project >>>>> moving forward as well. >>>>> >>>>> Please let me know what I can do to facilitate. >>>>> >>>>> Kind regards, Brian >>>>> >>>>> On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: >>>>> >>>>>> The problem with com.mycila.maven-license-plugin:maven-license-plugin >>>>>> as far as i remember is that is not yet published in central maven >>>>>> repository, so it cannot be used without adding their repo. in the >>>>> pom.xml >>>>>> which is a problem if you are trying to get your project deployed in OSS >>>>>> Sonatype. >>>>>> >>>>>> On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < >>>>>> [hidden email]> wrote: >>>>>> >>>>>>> Hi Brian, >>>>>>> >>>>>>> The main user of JUnit in production is WicketTester. >>>>>>> >>>>>>> About ApacheLicenceTest - Jeremy tried to replace it with >>>>>>> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >>>>>>> didn't finish it. >>>>>>> >>>>>>> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< >>>>> http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >>>>>>> wrote: >>>>>>> >>>>>>>> oic, there's a ApacheLicenseHeaderTest in every project. >>>>>>>> >>>>>>>> I'm in the process of isolating the junit.framework package to a test >>>>>>> dependency so JUnit is not a dependency in production code. If it were >>>>> made >>>>>>> into a plugin, the instances of per-project ApacheLicenseHeader >>>>>>> configuration would need to come from the POM. That's kind of where it >>>>>>> belongs (it's part of the build, after all), but it could easily be made >>>>>>> into a configuration file that resides in each project to keep the POMs >>>>>>> clean. >>>>>>>> >>>>>>>> Failing that, creating a separate module to contain o.a.w.util.license >>>>>>> that is a test scope dependency would be a last resort. >>>>>>>> >>>>>>>> I'm going to go ahead and create a plugin that reads a configuration >>>>> file >>>>>>> in each project. Some of the configurations are lengthy >>>>>>> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be >>>>> a >>>>>>> mess in the pom. >>>>>>>> >>>>>>>> >>>>>>>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >>>>>>>> >>>>>>>>> Hi all, >>>>>>>>> >>>>>>>>> Does anyone know why org.apache.wicket.util.license is in >>>>> wicket-util's >>>>>>> production source directory? I'm guessing it has something to do with >>>>> the >>>>>>> desire to get the license plugin to fire every time a build is made, but >>>>> if >>>>>>> that's the case, it would be better handled as a Maven plugin. It's not >>>>> a >>>>>>> test and it's not a part of any public API. >>>>>>>>> >>>>>>>>> I'm happy to create a plugin if that's the case, please let me know. >>>>>>>>> >>>>>>>>> Cheers, Brian >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Martin Grigorov >>>>>>> jWeekend >>>>>>> Training, Consulting, Development >>>>>>> http://jWeekend.com >>>>>>> >>>>>>> >>>>>>> ------------------------------ >>>>>>> If you reply to this email, your message will be added to the discussion >>>>>>> below: >>>>>>> >>>>>>> >>>>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >>>>>>> To start a new topic under Apache Wicket, email >>>>>>> [hidden email] >>>>>>> To unsubscribe from Apache Wicket, click here< >>>>> . >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> >>>>>> JC >>>>>> >>>>>> >>>>>> -- >>>>>> View this message in context: >>>>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html >>>>>> Sent from the Forum for Wicket Core developers mailing list archive at >>>>> Nabble.com. >>>>> >>>>> >>> >>> >> > > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com |
Re: OT: Code quality (was Re: o.a.w.util.license package in production source folder?)
|
|
Yes, that will create a patch of the whole branch, which is already there.
On Aug 15, 2011, at 11:58 AM, Martin Grigorov wrote: > git diff myBranch..master > some.patch >> >> On Aug 15, 2011, at 11:46 AM, Igor Vaynberg wrote: >> >>> can we have a patch that changes those places to use Args.*? :) >>> >>> -igor >>> >>> On Mon, Aug 15, 2011 at 7:50 AM, Brian Topping <[hidden email]> wrote: >>>> Hi guys, >>>> >>>> Just to provide some personal perspective and it's somewhat off-topic... there's always a lot of things we can work around here (code can always be compensated for with more code), but I think there is a responsibility with all code to "leave it cleaner than you found it". To say it differently, to me, any amount of effort today to keep things clean is worth it, because tomorrow (with additional code thrown on), it may take twice as long to undo it and we may not have options to work around the problem any longer (thus forcing that we cannot avoid cleaning it up with twice the investment). >>>> >>>> When I looked at the actual usages of JUnit, it was primarily on junit.framework.Assert in about three or four random files, when in fact the standing pattern is to use o.a.w.util.lang.Args or throw an IllegalStateException if there is a problem with incomplete initialization. >>>> >>>> In this case, removing JUnit as a dependency from util in fact improves the code, and in the process does not bury a dependency even deeper. In fact, there was a comment in one of the POMs alluding to the question of why JUnit was a runtime dependency. I don't think I am alone in believing that it should have been removed. This doesn't answer to o.a.w.util.tester.WicketTester, but that's better answered in Martin's email. >>>> >>>> Cheers and thanks, >>>> >>>> Brian >>>> >>>> On Aug 15, 2011, at 2:44 AM, Andreas Pieber wrote: >>>> >>>>> Hey guys, >>>>> >>>>> I just want to jump in here. While I think it a good idea to check license >>>>> headers via a plugin instead of a junit tests this is not a "no-go" for the >>>>> osgification. There are various libs out there importing org.junit... in the >>>>> compile phase instead of the test-phase (although not required). At >>>>> Servicemix such libs are typically wrapped using the ;optional:=true >>>>> attribute. Since junit is not required at runtime I think we can go the same >>>>> way for wicket here. >>>>> >>>>> WDYT? >>>>> >>>>> Kind regards, >>>>> Andreas >>>>> >>>>> On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: >>>>> >>>>>> Hi guys, thanks for the responses. The repository issue (as well as an >>>>>> unknown about outside plugins) was a concern, part of why I started a custom >>>>>> plugin. But if folks are comfortable with it, I think it's the right way to >>>>>> go. It's used in Brix and it's been very robust and convenient. >>>>>> >>>>>> I created a branch at >>>>>> https://github.com/topping/wicket/tree/myclila-plugin containing the >>>>>> changes. There are a lot of them and it took most of the day to get it >>>>>> right. The plugin expects the license header to be formatted slightly >>>>>> differently (for instance using "/**" instead of "/*" to start a Java >>>>>> header). Their site suggests using <aggregation>, but that results in all >>>>>> the configuration being in the parent POM, something that isn't very good >>>>>> encapsulation of configuration. So I broke it out between projects so it's >>>>>> easier to maintain. >>>>>> >>>>>> As for the specific excludes, I may not have precisely the same excludes >>>>>> that the old test cases had. I started by copying them to the best of my >>>>>> perception, then tuned them for the tests (which seems to be the most >>>>>> sensitive aspect). Can anyone review the patch to see if there are any >>>>>> obvious mistakes? >>>>>> >>>>>> If not, it would be very helpful for the OSGi effort if we could get this >>>>>> patch applied. Removing the dependency on JUnit from wicket-util is pretty >>>>>> important to the effort, and I think this provides benefits to the project >>>>>> moving forward as well. >>>>>> >>>>>> Please let me know what I can do to facilitate. >>>>>> >>>>>> Kind regards, Brian >>>>>> >>>>>> On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: >>>>>> >>>>>>> The problem with com.mycila.maven-license-plugin:maven-license-plugin >>>>>>> as far as i remember is that is not yet published in central maven >>>>>>> repository, so it cannot be used without adding their repo. in the >>>>>> pom.xml >>>>>>> which is a problem if you are trying to get your project deployed in OSS >>>>>>> Sonatype. >>>>>>> >>>>>>> On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < >>>>>>> [hidden email]> wrote: >>>>>>> >>>>>>>> Hi Brian, >>>>>>>> >>>>>>>> The main user of JUnit in production is WicketTester. >>>>>>>> >>>>>>>> About ApacheLicenceTest - Jeremy tried to replace it with >>>>>>>> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >>>>>>>> didn't finish it. >>>>>>>> >>>>>>>> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< >>>>>> http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> oic, there's a ApacheLicenseHeaderTest in every project. >>>>>>>>> >>>>>>>>> I'm in the process of isolating the junit.framework package to a test >>>>>>>> dependency so JUnit is not a dependency in production code. If it were >>>>>> made >>>>>>>> into a plugin, the instances of per-project ApacheLicenseHeader >>>>>>>> configuration would need to come from the POM. That's kind of where it >>>>>>>> belongs (it's part of the build, after all), but it could easily be made >>>>>>>> into a configuration file that resides in each project to keep the POMs >>>>>>>> clean. >>>>>>>>> >>>>>>>>> Failing that, creating a separate module to contain o.a.w.util.license >>>>>>>> that is a test scope dependency would be a last resort. >>>>>>>>> >>>>>>>>> I'm going to go ahead and create a plugin that reads a configuration >>>>>> file >>>>>>>> in each project. Some of the configurations are lengthy >>>>>>>> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be >>>>>> a >>>>>>>> mess in the pom. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >>>>>>>>> >>>>>>>>>> Hi all, >>>>>>>>>> >>>>>>>>>> Does anyone know why org.apache.wicket.util.license is in >>>>>> wicket-util's >>>>>>>> production source directory? I'm guessing it has something to do with >>>>>> the >>>>>>>> desire to get the license plugin to fire every time a build is made, but >>>>>> if >>>>>>>> that's the case, it would be better handled as a Maven plugin. It's not >>>>>> a >>>>>>>> test and it's not a part of any public API. >>>>>>>>>> >>>>>>>>>> I'm happy to create a plugin if that's the case, please let me know. >>>>>>>>>> >>>>>>>>>> Cheers, Brian >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Martin Grigorov >>>>>>>> jWeekend >>>>>>>> Training, Consulting, Development >>>>>>>> http://jWeekend.com >>>>>>>> >>>>>>>> >>>>>>>> ------------------------------ >>>>>>>> If you reply to this email, your message will be added to the discussion >>>>>>>> below: >>>>>>>> >>>>>>>> >>>>>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >>>>>>>> To start a new topic under Apache Wicket, email >>>>>>>> [hidden email] >>>>>>>> To unsubscribe from Apache Wicket, click here< >>>>>> . >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> >>>>>>> JC >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> View this message in context: >>>>>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html >>>>>>> Sent from the Forum for Wicket Core developers mailing list archive at >>>>>> Nabble.com. >>>>>> >>>>>> >>>> >>>> >>> >> >> > > > |
Re: o.a.w.util.license package in production source folder?
|
|
In reply to this post by Brian Topping
Hi Brian,
By unknown rules I meant whether there are some expectations by official Apache tools. I remember a tool named RAT which was used in 1.2.x and early 1.3.x releases but I know nothing more about it. .html files in test/java can be dynamic, i.e. WicketTestCase can save the output for a component/page and use it as "expected" result for future runs. So it is not possible to put the licence in all .html files. Another exception is usage of third party .js/.css files in wicket-examples - e.g. JQuery, Prototype.js I also like the idea of wicket-test.jar but a problem I see is that wicket-extensions also have **TesterHelper for LazyLoadPanel, and maybe wicket-test module will have to depend on -extensions ... Recently I tried to revive Matej's work on Wicket-Ajax NG and I tried to put all Ajax stuff in wicket-ajax.jar. It almost works but there were some problems which I have written somewhere but this is off topic here. On Mon, Aug 15, 2011 at 6:13 PM, Brian Topping <[hidden email]> wrote: > Hi Martin, comments inline. > > On Aug 15, 2011, at 3:50 AM, Martin Grigorov wrote: > >> Is it possible to configure the plugin to fail on unknown files ? > > There are no limitations of what we can do, even if we need to fork the mycila plugin to do it. :-) But if you mean "can the plugin as written deal with unknown files, the only way to do that would be to include everything (<include>**</include>) and then write an exclude that matches every file that should not be included. This will catch new files that are unknown in the way you mentioned. I didn't come to believe that the old system did this though, just that it handled the six or so types (java, js, xml, vm, css, and a couple others I don't remember ATM). > >> E.g. currently it excludes 'target/' folder and checks files with >> extensions .java, .xml, .html, .properties, .js, .css and .vm. Is it >> possible to add some global excludes for images and configure it to >> fail the verification if an unknown file is found. I.e. if there is >> src/main/java/org/apache/wicket/SomeFile.blah and it is not explicitly >> excluded then fail > > My experience with the old method of using unit tests to test that the source code had headers was not an exact process either. I had seen many files that did not have headers for no apparent reason. What I tried to do was find a "best fit" to the existing rules. With as many files as there are in the system, it would require an automated test rig to ensure that every file was being managed exactly like the old system did, and given that the old system had holes caught on visual inspection, it didn't seem that was time well-spent. > >> >> Additionally can you add some comments around why some resources are excluded ? >> I just saw wicket-core/pom.xml has: >> <plugin> >> 83 >> + <groupId>com.mycila.maven-license-plugin</groupId> >> 84 >> + <artifactId>maven-license-plugin</artifactId> >> 85 >> + <configuration> >> 86 >> + <header>${project.parent.basedir}/licenses/asf-license-header.txt</header> >> 87 >> + <excludes> >> 88 >> + <exclude>src/main/java/org/apache/wicket/**/*.properties</exclude> >> >> and I wonder why line 88 is there. > > Yes, line 88 is probably incorrect. I'll take a look at it. > >> I know current JUnit based tests also have some exclusions but I'm not >> sure whether they have explanation "why". > > The issue is with test data. For instance, a .html file in the test tree may not actually be used in the compilation of a test, it may be just for comparison that WicketTester created the final output that is correct. Adding a header to a file like that would cause the comparison (and the test) to fail. > > We could add that header to both the test input and test output, but I was trying to take the path of least impact. > >> >> On Mon, Aug 15, 2011 at 10:06 AM, Martin Grigorov <[hidden email]> wrote: >>> Hi, >>> >>> The change at https://github.com/topping/wicket/commit/8f08c2082f90fcd54a7e4734b892d13e610a47bf >>> looks OK to me but I'm not quite familiar with the unit test based >>> solution (the current one). I'm not sure whether there are some >>> (unknown for me) rules like specific formatting which may become >>> broken now. > > This doesn't seem to be the case. I'm committed to supporting the change that I made here though, so whatever minor changes are necessary, I'm happy to have issues assigned to me. > >>> >>> I guess Andreas' solution will be needed at least for wicket-core >>> because as I said (Base)WicketTester depends on JUnit - both compile >>> and runtime. > > Yes, the best practice in this case is to have a test module containing test code that has a runtime dependency on a library such as JUnit. There was a huge amount of changes to get this in, and I overlooked that o.a.w.util.tester is in core. Having it remain that way would cause the entire body of work to be pretty, but not very useful to it's original end. > > Are there practical reasons that o.a.w.util.tester is in core? > > <snip/> > > Cheers, Brian -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com |
Re: o.a.w.util.license package in production source folder?
|
|
Thanks for the feedback, Martin.
I also ran into the issues around .js testing and the dynamic .html expected output. In the latter case, I didn't dive deeper to realize that's what I was looking at, but I didn't pursue it either. Regarding the test jar(s), there is also o.a.w.extensions.ajax.markup.html.AjaxLazyLoadPanelTester and may be others I haven't gotten to yet. It seems like the right thing to do is to create a wicket-extensions-test (which has a dependency on wicket-test). This is not an ideal pattern, but part of the issue with the module structure of extensions not being in core. And if wicket-extensions were a standalone project somewhere else on the internet, we'd still need a wicket-extensions-test, so I don't think this is a degenerate case. I'll build out the latter with two test modules and we can take a look at it. Back to the topic of the license work, is there anything I can do to help prove / disprove the work I've done? I'm happy to do deeper research anywhere it's necessary, the bulk of the work on that is already done so it doesn't make sense to slow down now. On Aug 15, 2011, at 12:10 PM, Martin Grigorov wrote: > Hi Brian, > > By unknown rules I meant whether there are some expectations by > official Apache tools. I remember a tool named RAT which was used in > 1.2.x and early 1.3.x releases but I know nothing more about it. > > .html files in test/java can be dynamic, i.e. WicketTestCase can save > the output for a component/page and use it as "expected" result for > future runs. > So it is not possible to put the licence in all .html files. > > Another exception is usage of third party .js/.css files in > wicket-examples - e.g. JQuery, Prototype.js > > I also like the idea of wicket-test.jar but a problem I see is that > wicket-extensions also have **TesterHelper for LazyLoadPanel, and > maybe wicket-test module will have to depend on -extensions ... > > Recently I tried to revive Matej's work on Wicket-Ajax NG and I tried > to put all Ajax stuff in wicket-ajax.jar. It almost works but there > were some problems which I have written somewhere but this is off > topic here. > > On Mon, Aug 15, 2011 at 6:13 PM, Brian Topping <[hidden email]> wrote: >> Hi Martin, comments inline. >> >> On Aug 15, 2011, at 3:50 AM, Martin Grigorov wrote: >> >>> Is it possible to configure the plugin to fail on unknown files ? >> >> There are no limitations of what we can do, even if we need to fork the mycila plugin to do it. :-) But if you mean "can the plugin as written deal with unknown files, the only way to do that would be to include everything (<include>**</include>) and then write an exclude that matches every file that should not be included. This will catch new files that are unknown in the way you mentioned. I didn't come to believe that the old system did this though, just that it handled the six or so types (java, js, xml, vm, css, and a couple others I don't remember ATM). >> >>> E.g. currently it excludes 'target/' folder and checks files with >>> extensions .java, .xml, .html, .properties, .js, .css and .vm. Is it >>> possible to add some global excludes for images and configure it to >>> fail the verification if an unknown file is found. I.e. if there is >>> src/main/java/org/apache/wicket/SomeFile.blah and it is not explicitly >>> excluded then fail >> >> My experience with the old method of using unit tests to test that the source code had headers was not an exact process either. I had seen many files that did not have headers for no apparent reason. What I tried to do was find a "best fit" to the existing rules. With as many files as there are in the system, it would require an automated test rig to ensure that every file was being managed exactly like the old system did, and given that the old system had holes caught on visual inspection, it didn't seem that was time well-spent. >> >>> >>> Additionally can you add some comments around why some resources are excluded ? >>> I just saw wicket-core/pom.xml has: >>> <plugin> >>> 83 >>> + <groupId>com.mycila.maven-license-plugin</groupId> >>> 84 >>> + <artifactId>maven-license-plugin</artifactId> >>> 85 >>> + <configuration> >>> 86 >>> + <header>${project.parent.basedir}/licenses/asf-license-header.txt</header> >>> 87 >>> + <excludes> >>> 88 >>> + <exclude>src/main/java/org/apache/wicket/**/*.properties</exclude> >>> >>> and I wonder why line 88 is there. >> >> Yes, line 88 is probably incorrect. I'll take a look at it. >> >>> I know current JUnit based tests also have some exclusions but I'm not >>> sure whether they have explanation "why". >> >> The issue is with test data. For instance, a .html file in the test tree may not actually be used in the compilation of a test, it may be just for comparison that WicketTester created the final output that is correct. Adding a header to a file like that would cause the comparison (and the test) to fail. >> >> We could add that header to both the test input and test output, but I was trying to take the path of least impact. >> >>> >>> On Mon, Aug 15, 2011 at 10:06 AM, Martin Grigorov <[hidden email]> wrote: >>>> Hi, >>>> >>>> The change at https://github.com/topping/wicket/commit/8f08c2082f90fcd54a7e4734b892d13e610a47bf >>>> looks OK to me but I'm not quite familiar with the unit test based >>>> solution (the current one). I'm not sure whether there are some >>>> (unknown for me) rules like specific formatting which may become >>>> broken now. >> >> This doesn't seem to be the case. I'm committed to supporting the change that I made here though, so whatever minor changes are necessary, I'm happy to have issues assigned to me. >> >>>> >>>> I guess Andreas' solution will be needed at least for wicket-core >>>> because as I said (Base)WicketTester depends on JUnit - both compile >>>> and runtime. >> >> Yes, the best practice in this case is to have a test module containing test code that has a runtime dependency on a library such as JUnit. There was a huge amount of changes to get this in, and I overlooked that o.a.w.util.tester is in core. Having it remain that way would cause the entire body of work to be pretty, but not very useful to it's original end. >> >> Are there practical reasons that o.a.w.util.tester is in core? >> >> <snip/> >> >> Cheers, Brian > > > > -- > Martin Grigorov > jWeekend > Training, Consulting, Development > http://jWeekend.com > |
Re: o.a.w.util.license package in production source folder?
Administrator
|
I'm not sure why all this effort is going into something that works
really well for us, and has worked for years. I don't see any benefit from using an obscure maven plugin, while we have a test framework setup that works. Sounds like make work to me. As far as the junit dependency: wicket tester requires it, and we ship wicket tester in core. I don't want wicket tester going to a different JAR as I consider it an essential tool in Wicket development. Especially since OSGi should have some way of excluding junit should you want to do that. I don't have an objection to marking junit optional though. I don't see an argument that we should make life for 99% of wicket users more difficult, as well as our own lives, for just one OSGi dependency. Martijn On Mon, Aug 15, 2011 at 9:39 AM, Brian Topping <[hidden email]> wrote: > Thanks for the feedback, Martin. > > I also ran into the issues around .js testing and the dynamic .html expected output. In the latter case, I didn't dive deeper to realize that's what I was looking at, but I didn't pursue it either. > > Regarding the test jar(s), there is also o.a.w.extensions.ajax.markup.html.AjaxLazyLoadPanelTester and may be others I haven't gotten to yet. It seems like the right thing to do is to create a wicket-extensions-test (which has a dependency on wicket-test). This is not an ideal pattern, but part of the issue with the module structure of extensions not being in core. And if wicket-extensions were a standalone project somewhere else on the internet, we'd still need a wicket-extensions-test, so I don't think this is a degenerate case. > > I'll build out the latter with two test modules and we can take a look at it. > > Back to the topic of the license work, is there anything I can do to help prove / disprove the work I've done? I'm happy to do deeper research anywhere it's necessary, the bulk of the work on that is already done so it doesn't make sense to slow down now. > > On Aug 15, 2011, at 12:10 PM, Martin Grigorov wrote: > >> Hi Brian, >> >> By unknown rules I meant whether there are some expectations by >> official Apache tools. I remember a tool named RAT which was used in >> 1.2.x and early 1.3.x releases but I know nothing more about it. >> >> .html files in test/java can be dynamic, i.e. WicketTestCase can save >> the output for a component/page and use it as "expected" result for >> future runs. >> So it is not possible to put the licence in all .html files. >> >> Another exception is usage of third party .js/.css files in >> wicket-examples - e.g. JQuery, Prototype.js >> >> I also like the idea of wicket-test.jar but a problem I see is that >> wicket-extensions also have **TesterHelper for LazyLoadPanel, and >> maybe wicket-test module will have to depend on -extensions ... >> >> Recently I tried to revive Matej's work on Wicket-Ajax NG and I tried >> to put all Ajax stuff in wicket-ajax.jar. It almost works but there >> were some problems which I have written somewhere but this is off >> topic here. >> >> On Mon, Aug 15, 2011 at 6:13 PM, Brian Topping <[hidden email]> wrote: >>> Hi Martin, comments inline. >>> >>> On Aug 15, 2011, at 3:50 AM, Martin Grigorov wrote: >>> >>>> Is it possible to configure the plugin to fail on unknown files ? >>> >>> There are no limitations of what we can do, even if we need to fork the mycila plugin to do it. :-) But if you mean "can the plugin as written deal with unknown files, the only way to do that would be to include everything (<include>**</include>) and then write an exclude that matches every file that should not be included. This will catch new files that are unknown in the way you mentioned. I didn't come to believe that the old system did this though, just that it handled the six or so types (java, js, xml, vm, css, and a couple others I don't remember ATM). >>> >>>> E.g. currently it excludes 'target/' folder and checks files with >>>> extensions .java, .xml, .html, .properties, .js, .css and .vm. Is it >>>> possible to add some global excludes for images and configure it to >>>> fail the verification if an unknown file is found. I.e. if there is >>>> src/main/java/org/apache/wicket/SomeFile.blah and it is not explicitly >>>> excluded then fail >>> >>> My experience with the old method of using unit tests to test that the source code had headers was not an exact process either. I had seen many files that did not have headers for no apparent reason. What I tried to do was find a "best fit" to the existing rules. With as many files as there are in the system, it would require an automated test rig to ensure that every file was being managed exactly like the old system did, and given that the old system had holes caught on visual inspection, it didn't seem that was time well-spent. >>> >>>> >>>> Additionally can you add some comments around why some resources are excluded ? >>>> I just saw wicket-core/pom.xml has: >>>> <plugin> >>>> 83 >>>> + <groupId>com.mycila.maven-license-plugin</groupId> >>>> 84 >>>> + <artifactId>maven-license-plugin</artifactId> >>>> 85 >>>> + <configuration> >>>> 86 >>>> + <header>${project.parent.basedir}/licenses/asf-license-header.txt</header> >>>> 87 >>>> + <excludes> >>>> 88 >>>> + <exclude>src/main/java/org/apache/wicket/**/*.properties</exclude> >>>> >>>> and I wonder why line 88 is there. >>> >>> Yes, line 88 is probably incorrect. I'll take a look at it. >>> >>>> I know current JUnit based tests also have some exclusions but I'm not >>>> sure whether they have explanation "why". >>> >>> The issue is with test data. For instance, a .html file in the test tree may not actually be used in the compilation of a test, it may be just for comparison that WicketTester created the final output that is correct. Adding a header to a file like that would cause the comparison (and the test) to fail. >>> >>> We could add that header to both the test input and test output, but I was trying to take the path of least impact. >>> >>>> >>>> On Mon, Aug 15, 2011 at 10:06 AM, Martin Grigorov <[hidden email]> wrote: >>>>> Hi, >>>>> >>>>> The change at https://github.com/topping/wicket/commit/8f08c2082f90fcd54a7e4734b892d13e610a47bf >>>>> looks OK to me but I'm not quite familiar with the unit test based >>>>> solution (the current one). I'm not sure whether there are some >>>>> (unknown for me) rules like specific formatting which may become >>>>> broken now. >>> >>> This doesn't seem to be the case. I'm committed to supporting the change that I made here though, so whatever minor changes are necessary, I'm happy to have issues assigned to me. >>> >>>>> >>>>> I guess Andreas' solution will be needed at least for wicket-core >>>>> because as I said (Base)WicketTester depends on JUnit - both compile >>>>> and runtime. >>> >>> Yes, the best practice in this case is to have a test module containing test code that has a runtime dependency on a library such as JUnit. There was a huge amount of changes to get this in, and I overlooked that o.a.w.util.tester is in core. Having it remain that way would cause the entire body of work to be pretty, but not very useful to it's original end. >>> >>> Are there practical reasons that o.a.w.util.tester is in core? >>> >>> <snip/> >>> >>> Cheers, Brian >> >> >> >> -- >> Martin Grigorov >> jWeekend >> Training, Consulting, Development >> http://jWeekend.com >> > > -- Become a Wicket expert, learn from the best: http://wicketinaction.com |
Re: o.a.w.util.license package in production source folder?
|
|
On Aug 15, 2011, at 10:21 PM, Martijn Dashorst wrote:
> I'm not sure why all this effort is going into something that works > really well for us, and has worked for years. I don't see any benefit > from using an obscure maven plugin, while we have a test framework > setup that works. Sounds like make work to me. Not at all! I never noticed it before, that my projects have a transitive dependency on JUnit. It's a dependency that I don't see other projects I work with depending on, and it's probably a dependency that will never be made into a bundle. It's only being used for Assert in a few places, as I wrote earlier. Assertions are for testing invariants (assumptions that must be true in order for the code to continue), and it is assumed that situations where the invariants fail have been caught as coding errors during development and testing. Letting JUnit Assert throw unspecified RuntimeExceptions obfuscates what's actually going on. It's just a bad idea for a lot of reasons. Regarding the obscurity of the Maven plugin, it's been in Brix for years and we've had no problem with it at all. In fact, it's a lot more convenient because it will can be re-invoked to automatically add proper headers to missing files after they are found. It was mentioned during the week that Jeremy was working on changing things to use this plugin and ran out of time. It seemed like an idea that had already been vetted by a number of folks, so I went that direction. I can't imagine that he would have undertaken that work as a committer if there was a chance it would be rejected like that. Jeremy? > As far as the junit dependency: wicket tester requires it, and we ship > wicket tester in core. I don't want wicket tester going to a different > JAR as I consider it an essential tool in Wicket development. Of course it is, but does it ever get used at runtime? I'm trying to be constructive here, don't you believe test code should be in test modules and not bundled in to production artifacts? How does one additional test scope dependency sound so difficult or complex? > Especially since OSGi should have some way of excluding junit should > you want to do that. I don't have an objection to marking junit > optional though. JUnit can't be optional or excluded if there is code that depends on it. OSGi is still Java. :-) > > I don't see an argument that we should make life for 99% of wicket > users more difficult, as well as our own lives, for just one OSGi > dependency. I guess I don't understand the difficulty. If it has to do with an extra dependency, I'd merely ask if 99% of Wicket users use Hibernate, shouldn't Hibernate be a default dependency? The goal is to help improve Wicket, a framework that has been important to a lot of us for years. There's also a lot of new interest in the OSGi community around Wicket, and a lot of smart people there that would be great to have associated with any project. I would *never* advocate that Wicket should become harder to use for J2EE users, the group of people that would be using OSGi is not nearly large enough to warrant that. And if I was just saying that, I wouldn't have spent days digging into resolving this issue. But if Wicket remains hard to use for OSGi users, it's a bad sign in a lot of ways. Everything happens for a reason in the end, right? :-) > > Martijn > > On Mon, Aug 15, 2011 at 9:39 AM, Brian Topping <[hidden email]> wrote: >> Thanks for the feedback, Martin. >> >> I also ran into the issues around .js testing and the dynamic .html expected output. In the latter case, I didn't dive deeper to realize that's what I was looking at, but I didn't pursue it either. >> >> Regarding the test jar(s), there is also o.a.w.extensions.ajax.markup.html.AjaxLazyLoadPanelTester and may be others I haven't gotten to yet. It seems like the right thing to do is to create a wicket-extensions-test (which has a dependency on wicket-test). This is not an ideal pattern, but part of the issue with the module structure of extensions not being in core. And if wicket-extensions were a standalone project somewhere else on the internet, we'd still need a wicket-extensions-test, so I don't think this is a degenerate case. >> >> I'll build out the latter with two test modules and we can take a look at it. >> >> Back to the topic of the license work, is there anything I can do to help prove / disprove the work I've done? I'm happy to do deeper research anywhere it's necessary, the bulk of the work on that is already done so it doesn't make sense to slow down now. >> >> On Aug 15, 2011, at 12:10 PM, Martin Grigorov wrote: >> >>> Hi Brian, >>> >>> By unknown rules I meant whether there are some expectations by >>> official Apache tools. I remember a tool named RAT which was used in >>> 1.2.x and early 1.3.x releases but I know nothing more about it. >>> >>> .html files in test/java can be dynamic, i.e. WicketTestCase can save >>> the output for a component/page and use it as "expected" result for >>> future runs. >>> So it is not possible to put the licence in all .html files. >>> >>> Another exception is usage of third party .js/.css files in >>> wicket-examples - e.g. JQuery, Prototype.js >>> >>> I also like the idea of wicket-test.jar but a problem I see is that >>> wicket-extensions also have **TesterHelper for LazyLoadPanel, and >>> maybe wicket-test module will have to depend on -extensions ... >>> >>> Recently I tried to revive Matej's work on Wicket-Ajax NG and I tried >>> to put all Ajax stuff in wicket-ajax.jar. It almost works but there >>> were some problems which I have written somewhere but this is off >>> topic here. >>> >>> On Mon, Aug 15, 2011 at 6:13 PM, Brian Topping <[hidden email]> wrote: >>>> Hi Martin, comments inline. >>>> >>>> On Aug 15, 2011, at 3:50 AM, Martin Grigorov wrote: >>>> >>>>> Is it possible to configure the plugin to fail on unknown files ? >>>> >>>> There are no limitations of what we can do, even if we need to fork the mycila plugin to do it. :-) But if you mean "can the plugin as written deal with unknown files, the only way to do that would be to include everything (<include>**</include>) and then write an exclude that matches every file that should not be included. This will catch new files that are unknown in the way you mentioned. I didn't come to believe that the old system did this though, just that it handled the six or so types (java, js, xml, vm, css, and a couple others I don't remember ATM). >>>> >>>>> E.g. currently it excludes 'target/' folder and checks files with >>>>> extensions .java, .xml, .html, .properties, .js, .css and .vm. Is it >>>>> possible to add some global excludes for images and configure it to >>>>> fail the verification if an unknown file is found. I.e. if there is >>>>> src/main/java/org/apache/wicket/SomeFile.blah and it is not explicitly >>>>> excluded then fail >>>> >>>> My experience with the old method of using unit tests to test that the source code had headers was not an exact process either. I had seen many files that did not have headers for no apparent reason. What I tried to do was find a "best fit" to the existing rules. With as many files as there are in the system, it would require an automated test rig to ensure that every file was being managed exactly like the old system did, and given that the old system had holes caught on visual inspection, it didn't seem that was time well-spent. >>>> >>>>> >>>>> Additionally can you add some comments around why some resources are excluded ? >>>>> I just saw wicket-core/pom.xml has: >>>>> <plugin> >>>>> 83 >>>>> + <groupId>com.mycila.maven-license-plugin</groupId> >>>>> 84 >>>>> + <artifactId>maven-license-plugin</artifactId> >>>>> 85 >>>>> + <configuration> >>>>> 86 >>>>> + <header>${project.parent.basedir}/licenses/asf-license-header.txt</header> >>>>> 87 >>>>> + <excludes> >>>>> 88 >>>>> + <exclude>src/main/java/org/apache/wicket/**/*.properties</exclude> >>>>> >>>>> and I wonder why line 88 is there. >>>> >>>> Yes, line 88 is probably incorrect. I'll take a look at it. >>>> >>>>> I know current JUnit based tests also have some exclusions but I'm not >>>>> sure whether they have explanation "why". >>>> >>>> The issue is with test data. For instance, a .html file in the test tree may not actually be used in the compilation of a test, it may be just for comparison that WicketTester created the final output that is correct. Adding a header to a file like that would cause the comparison (and the test) to fail. >>>> >>>> We could add that header to both the test input and test output, but I was trying to take the path of least impact. >>>> >>>>> >>>>> On Mon, Aug 15, 2011 at 10:06 AM, Martin Grigorov <[hidden email]> wrote: >>>>>> Hi, >>>>>> >>>>>> The change at https://github.com/topping/wicket/commit/8f08c2082f90fcd54a7e4734b892d13e610a47bf >>>>>> looks OK to me but I'm not quite familiar with the unit test based >>>>>> solution (the current one). I'm not sure whether there are some >>>>>> (unknown for me) rules like specific formatting which may become >>>>>> broken now. >>>> >>>> This doesn't seem to be the case. I'm committed to supporting the change that I made here though, so whatever minor changes are necessary, I'm happy to have issues assigned to me. >>>> >>>>>> >>>>>> I guess Andreas' solution will be needed at least for wicket-core >>>>>> because as I said (Base)WicketTester depends on JUnit - both compile >>>>>> and runtime. >>>> >>>> Yes, the best practice in this case is to have a test module containing test code that has a runtime dependency on a library such as JUnit. There was a huge amount of changes to get this in, and I overlooked that o.a.w.util.tester is in core. Having it remain that way would cause the entire body of work to be pretty, but not very useful to it's original end. >>>> >>>> Are there practical reasons that o.a.w.util.tester is in core? >>>> >>>> <snip/> >>>> >>>> Cheers, Brian >>> >>> >>> >>> -- >>> Martin Grigorov >>> jWeekend >>> Training, Consulting, Development >>> http://jWeekend.com >>> >> >> > > > > -- > Become a Wicket expert, learn from the best: http://wicketinaction.com > |
Re: o.a.w.util.license package in production source folder?
|
|
On Tue, Aug 16, 2011 at 10:15 AM, Brian Topping <[hidden email]> wrote:
> On Aug 15, 2011, at 10:21 PM, Martijn Dashorst wrote: > >> I'm not sure why all this effort is going into something that works >> really well for us, and has worked for years. I don't see any benefit >> from using an obscure maven plugin, while we have a test framework >> setup that works. Sounds like make work to me. > > Not at all! I never noticed it before, that my projects have a transitive dependency on JUnit. It's a dependency that I don't see other projects I work with depending on, and it's probably a dependency that will never be made into a bundle. It's only being used for Assert in a few places, as I wrote earlier. This is one of the features that I liked the most in my early days in Wicket - its testing functionality. Many other frameworks don't provide such and thus don't require testing frameworks like JUnit. > > Assertions are for testing invariants (assumptions that must be true in order for the code to continue), and it is assumed that situations where the invariants fail have been caught as coding errors during development and testing. Letting JUnit Assert throw unspecified RuntimeExceptions obfuscates what's actually going on. It's just a bad idea for a lot of reasons. > > Regarding the obscurity of the Maven plugin, it's been in Brix for years and we've had no problem with it at all. In fact, it's a lot more convenient because it will can be re-invoked to automatically add proper headers to missing files after they are found. > > It was mentioned during the week that Jeremy was working on changing things to use this plugin and ran out of time. It seemed like an idea that had already been vetted by a number of folks, so I went that direction. I can't imagine that he would have undertaken that work as a committer if there was a chance it would be rejected like that. Jeremy? > >> As far as the junit dependency: wicket tester requires it, and we ship >> wicket tester in core. I don't want wicket tester going to a different >> JAR as I consider it an essential tool in Wicket development. choose between JUnit or TestNG and the solution was that all JUnit dependencies were moved out of BaseWicketTester to WicketTester and these users could make their own impl with TestNG. It seems this was broken in 1.5 because BaseWicketTester again have some JUnit imports. > > Of course it is, but does it ever get used at runtime? I'm trying to be constructive here, don't you believe test code should be in test modules and not bundled in to production artifacts? How does one additional test scope dependency sound so difficult or complex? WicketTester is mostly used for tests but some people use it to generate mail templates in production/runtime too. So it is not that optional. > >> Especially since OSGi should have some way of excluding junit should >> you want to do that. I don't have an objection to marking junit >> optional though. > > JUnit can't be optional or excluded if there is code that depends on it. OSGi is still Java. :-) Of course it can be optional. Unless you use WicketTester and Co. in runtime there is no need to register and start a bundle for JUnit. > >> >> I don't see an argument that we should make life for 99% of wicket >> users more difficult, as well as our own lives, for just one OSGi >> dependency. > > I guess I don't understand the difficulty. If it has to do with an extra dependency, I'd merely ask if 99% of Wicket users use Hibernate, shouldn't Hibernate be a default dependency? For Maven (Ivy/Gradle/...) users this is not a big deal. The problem comes for the rest (build systems w/o dependency management) - these users start to ask questions in the mailing lists why something is not there. IMO these users are the beginners, they don't think in Object Oriented way, nor in modular, they expect everything to be in one big .jar... > > The goal is to help improve Wicket, a framework that has been important to a lot of us for years. There's also a lot of new interest in the OSGi community around Wicket, and a lot of smart people there that would be great to have associated with any project. I would *never* advocate that Wicket should become harder to use for J2EE users, the group of people that would be using OSGi is not nearly large enough to warrant that. And if I was just saying that, I wouldn't have spent days digging into resolving this issue. But if Wicket remains hard to use for OSGi users, it's a bad sign in a lot of ways. > > Everything happens for a reason in the end, right? :-) For me extracting WicketTester and related classes in separate module is a plus. I'm just not happy that we try to do that in the last? RC of 1.5. About the Maven plugin - I have no experience with it so I can't say anything about it. All I know is that the current unit tests work and there are no open issues about them. And I don't see any problem they can cause to the OSGi related improvements. > >> >> Martijn >> >> On Mon, Aug 15, 2011 at 9:39 AM, Brian Topping <[hidden email]> wrote: >>> Thanks for the feedback, Martin. >>> >>> I also ran into the issues around .js testing and the dynamic .html expected output. In the latter case, I didn't dive deeper to realize that's what I was looking at, but I didn't pursue it either. >>> >>> Regarding the test jar(s), there is also o.a.w.extensions.ajax.markup.html.AjaxLazyLoadPanelTester and may be others I haven't gotten to yet. It seems like the right thing to do is to create a wicket-extensions-test (which has a dependency on wicket-test). This is not an ideal pattern, but part of the issue with the module structure of extensions not being in core. And if wicket-extensions were a standalone project somewhere else on the internet, we'd still need a wicket-extensions-test, so I don't think this is a degenerate case. >>> >>> I'll build out the latter with two test modules and we can take a look at it. >>> >>> Back to the topic of the license work, is there anything I can do to help prove / disprove the work I've done? I'm happy to do deeper research anywhere it's necessary, the bulk of the work on that is already done so it doesn't make sense to slow down now. >>> >>> On Aug 15, 2011, at 12:10 PM, Martin Grigorov wrote: >>> >>>> Hi Brian, >>>> >>>> By unknown rules I meant whether there are some expectations by >>>> official Apache tools. I remember a tool named RAT which was used in >>>> 1.2.x and early 1.3.x releases but I know nothing more about it. >>>> >>>> .html files in test/java can be dynamic, i.e. WicketTestCase can save >>>> the output for a component/page and use it as "expected" result for >>>> future runs. >>>> So it is not possible to put the licence in all .html files. >>>> >>>> Another exception is usage of third party .js/.css files in >>>> wicket-examples - e.g. JQuery, Prototype.js >>>> >>>> I also like the idea of wicket-test.jar but a problem I see is that >>>> wicket-extensions also have **TesterHelper for LazyLoadPanel, and >>>> maybe wicket-test module will have to depend on -extensions ... >>>> >>>> Recently I tried to revive Matej's work on Wicket-Ajax NG and I tried >>>> to put all Ajax stuff in wicket-ajax.jar. It almost works but there >>>> were some problems which I have written somewhere but this is off >>>> topic here. >>>> >>>> On Mon, Aug 15, 2011 at 6:13 PM, Brian Topping <[hidden email]> wrote: >>>>> Hi Martin, comments inline. >>>>> >>>>> On Aug 15, 2011, at 3:50 AM, Martin Grigorov wrote: >>>>> >>>>>> Is it possible to configure the plugin to fail on unknown files ? >>>>> >>>>> There are no limitations of what we can do, even if we need to fork the mycila plugin to do it. :-) But if you mean "can the plugin as written deal with unknown files, the only way to do that would be to include everything (<include>**</include>) and then write an exclude that matches every file that should not be included. This will catch new files that are unknown in the way you mentioned. I didn't come to believe that the old system did this though, just that it handled the six or so types (java, js, xml, vm, css, and a couple others I don't remember ATM). >>>>> >>>>>> E.g. currently it excludes 'target/' folder and checks files with >>>>>> extensions .java, .xml, .html, .properties, .js, .css and .vm. Is it >>>>>> possible to add some global excludes for images and configure it to >>>>>> fail the verification if an unknown file is found. I.e. if there is >>>>>> src/main/java/org/apache/wicket/SomeFile.blah and it is not explicitly >>>>>> excluded then fail >>>>> >>>>> My experience with the old method of using unit tests to test that the source code had headers was not an exact process either. I had seen many files that did not have headers for no apparent reason. What I tried to do was find a "best fit" to the existing rules. With as many files as there are in the system, it would require an automated test rig to ensure that every file was being managed exactly like the old system did, and given that the old system had holes caught on visual inspection, it didn't seem that was time well-spent. >>>>> >>>>>> >>>>>> Additionally can you add some comments around why some resources are excluded ? >>>>>> I just saw wicket-core/pom.xml has: >>>>>> <plugin> >>>>>> 83 >>>>>> + <groupId>com.mycila.maven-license-plugin</groupId> >>>>>> 84 >>>>>> + <artifactId>maven-license-plugin</artifactId> >>>>>> 85 >>>>>> + <configuration> >>>>>> 86 >>>>>> + <header>${project.parent.basedir}/licenses/asf-license-header.txt</header> >>>>>> 87 >>>>>> + <excludes> >>>>>> 88 >>>>>> + <exclude>src/main/java/org/apache/wicket/**/*.properties</exclude> >>>>>> >>>>>> and I wonder why line 88 is there. >>>>> >>>>> Yes, line 88 is probably incorrect. I'll take a look at it. >>>>> >>>>>> I know current JUnit based tests also have some exclusions but I'm not >>>>>> sure whether they have explanation "why". >>>>> >>>>> The issue is with test data. For instance, a .html file in the test tree may not actually be used in the compilation of a test, it may be just for comparison that WicketTester created the final output that is correct. Adding a header to a file like that would cause the comparison (and the test) to fail. >>>>> >>>>> We could add that header to both the test input and test output, but I was trying to take the path of least impact. >>>>> >>>>>> >>>>>> On Mon, Aug 15, 2011 at 10:06 AM, Martin Grigorov <[hidden email]> wrote: >>>>>>> Hi, >>>>>>> >>>>>>> The change at https://github.com/topping/wicket/commit/8f08c2082f90fcd54a7e4734b892d13e610a47bf >>>>>>> looks OK to me but I'm not quite familiar with the unit test based >>>>>>> solution (the current one). I'm not sure whether there are some >>>>>>> (unknown for me) rules like specific formatting which may become >>>>>>> broken now. >>>>> >>>>> This doesn't seem to be the case. I'm committed to supporting the change that I made here though, so whatever minor changes are necessary, I'm happy to have issues assigned to me. >>>>> >>>>>>> >>>>>>> I guess Andreas' solution will be needed at least for wicket-core >>>>>>> because as I said (Base)WicketTester depends on JUnit - both compile >>>>>>> and runtime. >>>>> >>>>> Yes, the best practice in this case is to have a test module containing test code that has a runtime dependency on a library such as JUnit. There was a huge amount of changes to get this in, and I overlooked that o.a.w.util.tester is in core. Having it remain that way would cause the entire body of work to be pretty, but not very useful to it's original end. >>>>> >>>>> Are there practical reasons that o.a.w.util.tester is in core? >>>>> >>>>> <snip/> >>>>> >>>>> Cheers, Brian >>>> >>>> >>>> >>>> -- >>>> Martin Grigorov >>>> jWeekend >>>> Training, Consulting, Development >>>> http://jWeekend.com >>>> >>> >>> >> >> >> >> -- >> Become a Wicket expert, learn from the best: http://wicketinaction.com >> > > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com |
Re: OT: Code quality (was Re: o.a.w.util.license package in production source folder?)
|
In reply to this post by Brian Topping
Did you guys consider working with pull requests on github? Works very
well in my experience. Eelco On Mon, Aug 15, 2011 at 9:08 AM, Brian Topping <[hidden email]> wrote: > Yes, that will create a patch of the whole branch, which is already there. > > On Aug 15, 2011, at 11:58 AM, Martin Grigorov wrote: > >> git diff myBranch..master > some.patch >>> >>> On Aug 15, 2011, at 11:46 AM, Igor Vaynberg wrote: >>> >>>> can we have a patch that changes those places to use Args.*? :) >>>> >>>> -igor >>>> >>>> On Mon, Aug 15, 2011 at 7:50 AM, Brian Topping <[hidden email]> wrote: >>>>> Hi guys, >>>>> >>>>> Just to provide some personal perspective and it's somewhat off-topic... there's always a lot of things we can work around here (code can always be compensated for with more code), but I think there is a responsibility with all code to "leave it cleaner than you found it". To say it differently, to me, any amount of effort today to keep things clean is worth it, because tomorrow (with additional code thrown on), it may take twice as long to undo it and we may not have options to work around the problem any longer (thus forcing that we cannot avoid cleaning it up with twice the investment). >>>>> >>>>> When I looked at the actual usages of JUnit, it was primarily on junit.framework.Assert in about three or four random files, when in fact the standing pattern is to use o.a.w.util.lang.Args or throw an IllegalStateException if there is a problem with incomplete initialization. >>>>> >>>>> In this case, removing JUnit as a dependency from util in fact improves the code, and in the process does not bury a dependency even deeper. In fact, there was a comment in one of the POMs alluding to the question of why JUnit was a runtime dependency. I don't think I am alone in believing that it should have been removed. This doesn't answer to o.a.w.util.tester.WicketTester, but that's better answered in Martin's email. >>>>> >>>>> Cheers and thanks, >>>>> >>>>> Brian >>>>> >>>>> On Aug 15, 2011, at 2:44 AM, Andreas Pieber wrote: >>>>> >>>>>> Hey guys, >>>>>> >>>>>> I just want to jump in here. While I think it a good idea to check license >>>>>> headers via a plugin instead of a junit tests this is not a "no-go" for the >>>>>> osgification. There are various libs out there importing org.junit... in the >>>>>> compile phase instead of the test-phase (although not required). At >>>>>> Servicemix such libs are typically wrapped using the ;optional:=true >>>>>> attribute. Since junit is not required at runtime I think we can go the same >>>>>> way for wicket here. >>>>>> >>>>>> WDYT? >>>>>> >>>>>> Kind regards, >>>>>> Andreas >>>>>> >>>>>> On Sun, Aug 14, 2011 at 22:24, Brian Topping <[hidden email]> wrote: >>>>>> >>>>>>> Hi guys, thanks for the responses. The repository issue (as well as an >>>>>>> unknown about outside plugins) was a concern, part of why I started a custom >>>>>>> plugin. But if folks are comfortable with it, I think it's the right way to >>>>>>> go. It's used in Brix and it's been very robust and convenient. >>>>>>> >>>>>>> I created a branch at >>>>>>> https://github.com/topping/wicket/tree/myclila-plugin containing the >>>>>>> changes. There are a lot of them and it took most of the day to get it >>>>>>> right. The plugin expects the license header to be formatted slightly >>>>>>> differently (for instance using "/**" instead of "/*" to start a Java >>>>>>> header). Their site suggests using <aggregation>, but that results in all >>>>>>> the configuration being in the parent POM, something that isn't very good >>>>>>> encapsulation of configuration. So I broke it out between projects so it's >>>>>>> easier to maintain. >>>>>>> >>>>>>> As for the specific excludes, I may not have precisely the same excludes >>>>>>> that the old test cases had. I started by copying them to the best of my >>>>>>> perception, then tuned them for the tests (which seems to be the most >>>>>>> sensitive aspect). Can anyone review the patch to see if there are any >>>>>>> obvious mistakes? >>>>>>> >>>>>>> If not, it would be very helpful for the OSGi effort if we could get this >>>>>>> patch applied. Removing the dependency on JUnit from wicket-util is pretty >>>>>>> important to the effort, and I think this provides benefits to the project >>>>>>> moving forward as well. >>>>>>> >>>>>>> Please let me know what I can do to facilitate. >>>>>>> >>>>>>> Kind regards, Brian >>>>>>> >>>>>>> On Aug 14, 2011, at 9:05 AM, jcgarciam wrote: >>>>>>> >>>>>>>> The problem with com.mycila.maven-license-plugin:maven-license-plugin >>>>>>>> as far as i remember is that is not yet published in central maven >>>>>>>> repository, so it cannot be used without adding their repo. in the >>>>>>> pom.xml >>>>>>>> which is a problem if you are trying to get your project deployed in OSS >>>>>>>> Sonatype. >>>>>>>> >>>>>>>> On Sun, Aug 14, 2011 at 4:54 AM, Martin Grigorov-4 [via Apache Wicket] < >>>>>>>> [hidden email]> wrote: >>>>>>>> >>>>>>>>> Hi Brian, >>>>>>>>> >>>>>>>>> The main user of JUnit in production is WicketTester. >>>>>>>>> >>>>>>>>> About ApacheLicenceTest - Jeremy tried to replace it with >>>>>>>>> com.mycila.maven-license-plugin:maven-license-plugin in 1.4.x but >>>>>>>>> didn't finish it. >>>>>>>>> >>>>>>>>> On Sun, Aug 14, 2011 at 6:04 AM, Brian Topping <[hidden email]< >>>>>>> http://user/SendEmail.jtp?type=node&node=3742539&i=0>> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> oic, there's a ApacheLicenseHeaderTest in every project. >>>>>>>>>> >>>>>>>>>> I'm in the process of isolating the junit.framework package to a test >>>>>>>>> dependency so JUnit is not a dependency in production code. If it were >>>>>>> made >>>>>>>>> into a plugin, the instances of per-project ApacheLicenseHeader >>>>>>>>> configuration would need to come from the POM. That's kind of where it >>>>>>>>> belongs (it's part of the build, after all), but it could easily be made >>>>>>>>> into a configuration file that resides in each project to keep the POMs >>>>>>>>> clean. >>>>>>>>>> >>>>>>>>>> Failing that, creating a separate module to contain o.a.w.util.license >>>>>>>>> that is a test scope dependency would be a last resort. >>>>>>>>>> >>>>>>>>>> I'm going to go ahead and create a plugin that reads a configuration >>>>>>> file >>>>>>>>> in each project. Some of the configurations are lengthy >>>>>>>>> (org.apache.wicket.util.license.ApacheLicenceHeaderTest). That would be >>>>>>> a >>>>>>>>> mess in the pom. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Aug 13, 2011, at 10:09 PM, Brian Topping wrote: >>>>>>>>>> >>>>>>>>>>> Hi all, >>>>>>>>>>> >>>>>>>>>>> Does anyone know why org.apache.wicket.util.license is in >>>>>>> wicket-util's >>>>>>>>> production source directory? I'm guessing it has something to do with >>>>>>> the >>>>>>>>> desire to get the license plugin to fire every time a build is made, but >>>>>>> if >>>>>>>>> that's the case, it would be better handled as a Maven plugin. It's not >>>>>>> a >>>>>>>>> test and it's not a part of any public API. >>>>>>>>>>> >>>>>>>>>>> I'm happy to create a plugin if that's the case, please let me know. >>>>>>>>>>> >>>>>>>>>>> Cheers, Brian >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Martin Grigorov >>>>>>>>> jWeekend >>>>>>>>> Training, Consulting, Development >>>>>>>>> http://jWeekend.com >>>>>>>>> >>>>>>>>> >>>>>>>>> ------------------------------ >>>>>>>>> If you reply to this email, your message will be added to the discussion >>>>>>>>> below: >>>>>>>>> >>>>>>>>> >>>>>>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742539.html >>>>>>>>> To start a new topic under Apache Wicket, email >>>>>>>>> [hidden email] >>>>>>>>> To unsubscribe from Apache Wicket, click here< >>>>>>> . >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> >>>>>>>> JC >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> View this message in context: >>>>>>> http://apache-wicket.1842946.n4.nabble.com/o-a-w-util-license-package-in-production-source-folder-tp3742291p3742824.html >>>>>>>> Sent from the Forum for Wicket Core developers mailing list archive at >>>>>>> Nabble.com. >>>>>>> >>>>>>> >>>>> >>>>> >>>> >>> >>> >> >> >> > > |
«
Return to Forum for Wicket Core developers
|
1 view|%1 views
Loading...
| Powered by Nabble | Edit this page |