[SUREFIRE-1954] move inner class ProviderList to upper level #393
Conversation
|
Hi @Tibor17 I used Plexus IoC ... because project use maven 3.0 and for JSR330 probably 3.1+ is required ... in next steps. |
|
Hi @slawekjaranowski , I am just having a look. |
| @@ -1,4 +1,4 @@ | |||
| package org.apache.maven.plugin.surefire; | |||
| package org.apache.maven.surefire.providerapi; | |||
There was a problem hiding this comment.
@slawekjaranowski Why the package has changed? There are probably many classes in the new package, right?
There was a problem hiding this comment.
I put all classes responsible for providers detecting in one package.
Before we have:
org.apache.maven.plugin.surefire.booterclient.ProviderDetector
which use
org.apache.maven.surefire.providerapi.ServiceLoader
and ProviderDetector was used only by AbstractSurefireMojo so one responsibility was in many package.
There was a problem hiding this comment.
yes, I see there are now 5 classes. LGTM
| @@ -22,7 +22,7 @@ | |||
| /** | |||
| * @author Kristian Rosenvold | |||
| */ | |||
| interface ConfigurableProviderInfo | |||
| public interface ConfigurableProviderInfo | |||
There was a problem hiding this comment.
@slawekjaranowski This interface is only used ASM class or you think that other project which extends the ASM class can instantiate it in the subclass of ASM class?
There was a problem hiding this comment.
before we have, in package: org.apache.maven.plugin.surefire
- interface
ConfigurableProviderInfoextendsProviderInfo - public interface
ProviderInfo
both used inAbstarctSurefireMojofor implementing inner class like xxxProviderInfo
Now are moved to providerapi and both must be public for technical purpose.
| * @author Kristian Rosenvold | ||
| */ | ||
| @Component( role = ProviderDetector.class ) | ||
| public final class ProviderDetector |
There was a problem hiding this comment.
Nicely extracted class. I want to ask about the modifiers public. Are they important?
There was a problem hiding this comment.
used in ASM from another package ...
| new JUnit4ProviderInfo( getJunitArtifact(), junitDepArtifact ), | ||
| new JUnit3ProviderInfo() ) | ||
| .resolve(); | ||
| return providerDetector.resolve( new DynamicProviderInfo( null ), |
There was a problem hiding this comment.
Why this line has changed? Previously the line was return new ProviderList( new DynamicProviderInfo( null ),.
There was a problem hiding this comment.
now providerDetector is component, so we use service method
Code from inner class ProviderList are in ProviderDetector
|
@slawekjaranowski |
| @@ -42,7 +45,8 @@ | |||
| * | |||
| * @since 2.20 | |||
| */ | |||
| public final class ServiceLoader | |||
| @Component( role = ServiceLoader.class ) | |||
| public class ServiceLoader | |||
There was a problem hiding this comment.
@slawekjaranowski Can this Component be also public final class as the other classes in this package? So that we indicate that the component class cannot be extended by the user in another project.
There was a problem hiding this comment.
I use mock ServiceLoader of in ProviderDetectorTest but final clases can't be mocked.
I can introduce interfaces for components and mock on interface will be posible.
There was a problem hiding this comment.
ah ok, so let it be, the tests with mock are more important
|
@slawekjaranowski |
|
Rebased |
|
Pls do not change the code conventions with another purpose of the PR because then the reviewer has more work to check every line. Some contributors use to add hacks along another purpose and they think we would not find it. It's better to have another PR for coding conventions. |
|
@slawekjaranowski gitbox is down, so i have to wait with your Jira ticket to close it. |
Before final change I did small refractor for easy testing by unit tests