Community
Participate
Working Groups
Our product has some types of projects that do not work well with Maven, so we'd like to have a way to prevent a user from converting to Maven those types of projects. In other words, we do not want to allow users to select the menu Configure -> Convert to Maven project if they select one project with any of those types. What I propose is the following: 1. Create an extension point in which products can declare a class that will validate if a given project can/should be converted to Maven 2. The wizard would interrogate all the declared validators. If at least one says that the project cannot be converted, the wizard displays the error message returned by the validator, and does not allow the user to finish the wizard. I'm willing to contribute this patch by Monday. But I'd like to hear opinions. Or maybe there is already a way to accomplish this in m2e? Thanks
Sounds reasonable. My only concern is that this extension point will have to be instantiated for all projects, which will trigger often unnecessary activation of the plugin that contributes the extension. Also, 1.2 is done at this point. 1.3 is the earliest we'll be able to include this, which may be released together with Juno SR2 or, more likely, with Kepler SR0.
Too bad it cannot be included in 1.2. But I will be providing a patch for 1.3 Thank you.
Created attachment 224933 [details] Proposed patch Attached is a patch that allows extenders to determine if a project should be converted or not. Besides, it allows extenders to also suggest the packaging used for a given project. For example, if the project being converted is an EAR project, an extender could implement this extension point and say that the default packaging used for this project is ear, instead of the current default (jar)
@Fred, this one is yours, please review when you have time.
Roberto, I had a look at the patch, here are my comments : - U/ITs are missing, please attach a separate patch for https://github.com/sonatype/m2e-core-tests - in org.eclipse.m2e.core/schema/conversionEnabler.exsd meta.section type="since" says 1.7 instead of 1.3 - The getPackagingTypes handling is wrong IMHO : you only take the 1st packaging types from one single projectConversionEnabler, even if it returns several choices. Take a simple java project for instance, it could be converted to many different project types (sar, jar, some custom extension, ...) All the potential packagings from all projectConversionEnabler should be added to the packaging menu list, ordered by their projectConversionEnabler's weight - I would have sorted the projectConversionEnablers directly in ProjectConversionEnablerHelper.initialize - Speaking of ProjectConversionEnablerHelper, why didn't you add the new methods to (I)ProjectConversionManager instead?
Forgot about the canBeConverted method : if an IStatus.ERROR is returned, why display the wizard at all, since the user can't proceed with the conversion? Now that'd make more sense if you could *also* return warnings instead. In that case, I would expect the warning message to be displayed in the wizard, but it'd still let the user proceed with the conversion, at his own risks. We could then imagine the contributor plugin having some preferences where the user could select whether the maven conversion could be prevented completely or just discouraged.
(In reply to comment #5) > Roberto, I had a look at the patch, here are my comments : > - U/ITs are missing, please attach a separate patch for > https://github.com/sonatype/m2e-core-tests > - in org.eclipse.m2e.core/schema/conversionEnabler.exsd meta.section > type="since" says 1.7 instead of 1.3 > - The getPackagingTypes handling is wrong IMHO : you only take the 1st > packaging types from one single projectConversionEnabler, even if it returns > several choices. Take a simple java project for instance, it could be > converted to many different project types (sar, jar, some custom extension, > ...) All the potential packagings from all projectConversionEnabler should > be added to the packaging menu list, ordered by their > projectConversionEnabler's weight > - I would have sorted the projectConversionEnablers directly in > ProjectConversionEnablerHelper.initialize > - Speaking of ProjectConversionEnablerHelper, why didn't you add the new > methods to (I)ProjectConversionManager instead? Hi Fred, thank you for reviewing the patch. Regarding the tests, I will work on them. Regarding the change in the "since" field, I guess that value is somehow the default, and I did not noticed it. I will change it Regarding the getPackagingTypes handling, that was my initial intention: allow extenders to provide more than one packaging. But I then realized that the class for the wizard page does not expose a method for setting the list of available packaging options, it only lets to set the selected value. I can change that: I will add a new method to the wizard page to set the list of available packaging types. Regarding the sorting, I can do it in the initialize. And regarding why I did not add the methods of ProjectConversionEnablerHelper to (I)ProjectConversionManager, is because I did not want to add more methods to an interface, that could have been implemented by somebody else, and break their code. But if you believe it is safe to modify the (I)ProjectConversionManager classes, I can move the methods to that classes and get rid of ProjectConversionEnablerHelper.
(In reply to comment #6) > Forgot about the canBeConverted method : if an IStatus.ERROR is returned, > why display the wizard at all, since the user can't proceed with the > conversion? > > Now that'd make more sense if you could *also* return warnings instead. In > that case, I would expect the warning message to be displayed in the wizard, > but it'd still let the user proceed with the conversion, at his own risks. > We could then imagine the contributor plugin having some preferences where > the user could select whether the maven conversion could be prevented > completely or just discouraged. I wanted to display the wizard to have a consistent UI. If we do not display the wizard, we would need to display something else (maybe a dialog) to inform the user why the project cannot be converted. And regarding the warning, that is a good suggestion I can add.
Created attachment 225483 [details] Version 2 of proposed patch This is the second version of the original patch. I incorporated all Fred's suggestions, except the warning one. I believe it is a good suggestion, but given that it is out of the scope of the original use case reported here, I'd like to implement it later, if someone need it. At this point, I'm more interested in the error and in the packaging types.
Created attachment 225485 [details] Patch with new JUnits I'm attaching the JUnits that Fred requested.
(In reply to comment #10) > Created attachment 225485 [details] > Patch with new JUnits > > I'm attaching the JUnits that Fred requested. Please don't do things like assertTrue( actual ==/equals expected), use assertEquals(expected, actual) instead. If the assertion fails, Junit will then provide a useful message like "got 'result' but expected 'expected'" Use assert(Not)Null as well when needed.
Created attachment 225508 [details] Version 2 of new JUnits
(In reply to comment #11) > (In reply to comment #10) > > Created attachment 225485 [details] > > Patch with new JUnits > > > > I'm attaching the JUnits that Fred requested. > > Please don't do things like assertTrue( actual ==/equals expected), use > assertEquals(expected, actual) instead. If the assertion fails, Junit will > then provide a useful message like "got 'result' but expected 'expected'" > Use assert(Not)Null as well when needed. Thanks for the feedback. I attached a second version of the JUnits patch
Thanks for the patches Roberto. I opened IPZilla CQ, as the contribution is larger than 250L (9 files changed, 356 insertions(+), 7 deletions(-)) : https://dev.eclipse.org/ipzilla/show_bug.cgi?id=7004 (requires IPZilla access)
(In reply to comment #14) > Thanks for the patches Roberto. > I opened IPZilla CQ, as the contribution is larger than 250L (9 files > changed, 356 insertions(+), 7 deletions(-)) : > https://dev.eclipse.org/ipzilla/show_bug.cgi?id=7004 (requires IPZilla > access) Hi Fred, I see in the IPZilla a + next to the PMC. Is that what we are waiting?
means the PMC agreed to get the contribution in, now EMO needs to clear it from an IP perspective. Once they do that we can merge it to our code base.
Patches applied : http://git.eclipse.org/c/m2e/m2e-core.git/commit/?id=279c8cb44cb15ada20d06950eb2bd3f9bf47982e and https://github.com/sonatype/m2e-core-tests/commit/a13133c64486b8081072bcd04a6795e099a159ef Thanks Roberto. Wait for today's build at http://nexus.tesla.io:8081/nexus/content/sites/m2e.extras/m2e/1.3.0/N/LATEST/ to test your changes.
cleaning IP Log up
Moved to https://github.com/eclipse-m2e/m2e-core/issues/
