New Cohort Module: Change to signature of addMember

Development
#1

Between OpenMRS 2.0 and 2.1 the signature of the addMember(Integer) method of Cohort changes from:

public void addMember(Integer memberId) {
	getMemberIds().add(memberId);
}

to:

public boolean addMember(Integer memberId) {
     return this.addMembership(new CohortMembership(memberId));
 }

I thought we didn’t allow signature changes between point releases? Thoughts on whether it would be allowable to revert this change in 2.1.x and master branches?

It’s relevant for us because in the reporting module we are thinking of overriding the new behavior of Cohort and revert it back to a simple Integer set, for perform purposes, but we’d have to jump through a lot of hoops and/or start maintain pre-and-post 2.1.0 branches of reporting if we had to account for the signature change between the two versions.

I tried changing the method in the 2.1.x branch and tests still compile and run, so it appears that the boolean return value is not currently been used in core.

(As a side note, the removeMember(Integer) method was removed between 2.0 and 2.1, which seems wrong as well, but is less of an issue for us).

Thoughts?

fyi @mseaton @burke @darius

#2

(Just fixed the above to correctly show the change between the two versions of addMember())

#3

I agree that we shouldn’t have changed the method signature between 2.0 and 2.1 (even though adding a return type seems innocuous it actually breaks things because compiled code won’t find the same method).

I also agree that removeMember(Integer) should not have been removed, even if it has to throw an UnsupportedOperationException. Though I would presume that we can find a sufficiently-good behavior.

I think it’s okay to change the method signatures back in 2.1.x and do another maintenance release alongside the next reporting module release.