Comment 1 Sam Davis 2012-02-03 19:03:27 EST

Created attachment 210533 [details]
filtering suppport

Comment 2 Steffen Pingel 2012-02-09 21:43:52 EST

This looks good. Can you include a test case with the patch? It would also be nice to support an integer priority attribute for sorting the list of extensions.

Comment 3 Steffen Pingel 2012-02-12 14:37:22 EST

I have committed a simple test case for ExtensionPointReader that we can build on.

Comment 4 Sam Davis 2012-02-13 19:29:06 EST

Created attachment 210949 [details]
updated patch

I decided to allow double values for priority even though this should be discouraged, just in case someone gets into a situation where they need to insert something in between extensions with priority n and n+1.

Comment 5 Steffen Pingel 2012-02-15 12:36:47 EST

Thanks Sam! I have applied the patch to master.

Comment 6 Steffen Pingel 2012-02-17 18:55:08 EST

This tests keeps failing everyonce in a while:

pre. 
junit.framework.AssertionFailedError: expected:
<[org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest$P10ExtensionPointReaderExtensionImplementation@1f, org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest$P5ExtensionPointReaderExtensionImplementation@1f, org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest$ExtensionPointReaderExtensionImplementation@1f, org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest$PNegative5ExtensionPointReaderExtensionImplementation@1f]> 
but was:
<[org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest$ExtensionPointReaderExtensionImplementation@1f, org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest$P5ExtensionPointReaderExtensionImplementation@1f, org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest$PNegative5ExtensionPointReaderExtensionImplementation@1f, org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest$P10ExtensionPointReaderExtensionImplementation@1f]>
	at junit.framework.Assert.fail(Assert.java:47)
	at junit.framework.Assert.failNotEquals(Assert.java:283)
	at junit.framework.Assert.assertEquals(Assert.java:64)
	at junit.framework.Assert.assertEquals(Assert.java:71)
	at org.eclipse.mylyn.commons.tests.core.ExtensionPointReaderTest.testRead(ExtensionPointReaderTest.java:86)

Comment 7 Steffen Pingel 2012-02-17 19:14:03 EST

I have fixed the equals implementation of the test stub and renamed the default for the priority attribute from "extensionPriority" to "priority".

Comment 8 Steffen Pingel 2012-02-23 18:10:22 EST

The priority test was still failing on Java. I just noticed that the comparator implementation indeed does not look correct:

			if (p1 > p0) {
				return 1;
			} else if (p1 < -p0) {
				return -1;
			}
			
Sam, I assume the extra sign in p1 < -p0 was just accidental or am I missing something?

Comment 9 Sam Davis 2012-02-23 18:23:45 EST

Yes, that looks like an accident. That whole if statement should probably be replaced with return p1 - p0.

Comment 10 Steffen Pingel 2012-02-23 18:24:53 EST

Thanks. I have fixed it and unit tests are now passing on Java 1.7 as well.