Hi Mirko,
it looks to me the name of the rule is wrong with your described intension.
RequireContributorRoles would mean that a contributor requires a role
The role isn't mandatory, so I think you shouldn't use Required as prefix,
but maybe AllowedContributorRoles.
I assumed that contributors and developers are different object, but now I
see
public class Developer extends Contributor
That part could be reverted, although it looks a bit weird.
On Thu, 19 Apr 2012 23:13:51 +0200, Mirko Friedenhagen
<[email protected]> wrote:
Hello Robert,
a) maybe the intentions of RequireContributorRoles and
RequireDeveloperRoles are not clear:
- I did not want every developer or contributor to *have a role*, but
that certain roles are taken in a project, so the code checks that at
least one of the developers/contributors in a project has one of the
required roles. Say you always want to have someone to be the
"business engineer" in a project, so you define that this role is
taken at least by one of the contributors. So I changed the error
message to be:
"Found no %s representing role(s) '%s'", "developer|contributor",
rolesFromProperties
- Why did you pull down the getRolesFromProject method? It is
identical in both RequireContributorRoles and RequireDeveloperRoles
except of the input list :-).
b) In regards to RequirePropertyDiverges: I choose to compare against
a Xpp3Dom as plugins.get( MAVEN_ENFORCER_PLUGIN ).getConfiguration()
returns a Xpp3Dom which I otherwise would have had to parse again.
c) Unless someone wanted to extend the new rules, I like to minimize
visibility therefore I normally prefer the default (package) scope
instead of protected. This allows to write unit tests easily without
widening the API.
Regards Mirko
On Wed, Apr 18, 2012 at 23:00, Robert Scholte <[email protected]>
wrote:
Mirko,
some remarks:
IMO requireRole would mean that every developer/contributor requires at
least one role.
But the code checks if the used roles are allowed. So i'd prefer to
rename
those rules.
Don't you want to log the offending contributer/developer?
If some properties are required, I think you should verfy them first.
So if AbstractRequireRoles.requiredRoles (actually allowedRoles) is
empty or
RequirePropertyDiverges.property is empty the rule should fail
immediately.
The RequirePropertyDiverges looks like a pretty flat rule, so I don't
see
why need those Xpp3Dom objects, but I could be wrong here.
-Robert
ps. I've already made some changes to the code
________________________________
From: [email protected]
To: [email protected]
Date: Mon, 16 Apr 2012 21:07:07 +0000
Subject: RE: [mojo-dev] New release of extra-enforcer-rules?
I'll try to have a look at it this week.
Robert
From: [email protected]
Date: Sun, 15 Apr 2012 22:02:36 +0200
To: [email protected]
Subject: [mojo-dev] New release of extra-enforcer-rules?
Hello,
I have merged back my branch into the trunk which implements three new
rules for checking diverging properties from an ancestor project as
well as rules for checking certain roles for developers and
contributors are defined in a project. I have written unit tests as
well as invoker tests and am confident they should work.
Are there any plans for releasing extra-enforcer-rules-1.0-alpha-3?
Regards Mirko
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email
--
Using Opera's revolutionary email client: http://www.opera.com/mail/
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email
