Hi Myrle, Thanks for taking time out to review.
About those 2 last lines of code, I could update all pull requests by removing the ext.name and ext.year variables but you already started merging some... ;-). Cheers, Isaac Kamga. On Mon, Mar 5, 2018 at 12:57 PM, Myrle Krantz <my...@apache.org> wrote: > Hey Isaac, > > I'm reviewing your pull requests bit-by-bit, but I'm not going to be > able to get through them all today, because I've got some other stuff > I have to work on. > > What I've seen so far looks great though, so I've been merging them, > once I've tested the build on my local machine. > > One tiny niggle that I didn't feel was worth holding up the merge for: > the ext.name and ext.year variables aren't necessary anymore because > the header you're using doesn't address them. Those two lines can > eventually be deleted, even if they aren't hurting anyone now. > > Best Regards, > Myrle > > On Thu, Mar 1, 2018 at 8:38 PM, Isaac Kamga <isaac.ka...@mifos.org> wrote: > > Thanks a million Myrle and Phil. > > > > I followed your instructions and have updated the Pull Request with a new > > commit. > > > > At Your Service, > > Isaac Kamga. > > > > On Thu, Mar 1, 2018 at 7:49 PM, Myrle Krantz <my...@apache.org> wrote: > > > >> Oops. Do what Phil says. He knows better. > >> > >> Greets, > >> Myrle > >> > >> On Thu 1. Mar 2018 at 19:43 Phil Steitz <phil.ste...@gmail.com> wrote: > >> > >> > On 3/1/18 10:34 AM, Myrle Krantz wrote: > >> > > Hey Isaac, > >> > > > >> > > At a first glance, it looks good. May I ask though why you removed > >> > > the license parameter strictCheck? > >> > > > >> > > Regards, > >> > > Myrle > >> > > >> > The copyright statement should be removed entirely from the source > >> > header files and placed instead in NOTICE.txt. > >> > > >> > See http://www.apache.org/legal/src-headers.html > >> > > >> > Phil > >> > > > >> > > On Thu, Mar 1, 2018 at 5:27 PM, Isaac Kamga <isaac.ka...@mifos.org> > >> > wrote: > >> > >> Hi Myrle, > >> > >> > >> > >> I've just updated copyright information in fineract-cn-lang and > >> created > >> > a > >> > >> new pull request <https://github.com/apache/ > fineract-cn-lang/pull/4>. > >> > >> > >> > >> I patiently await your review and possible merger. > >> > >> > >> > >> At Your Service, > >> > >> Isaac Kamga. > >> > >> > >> > >> On Thu, Mar 1, 2018 at 5:05 PM, Isaac Kamga <isaac.ka...@mifos.org > > > >> > wrote: > >> > >> > >> > >>> Hi Myrle, > >> > >>> > >> > >>> Thanks a million for your advice and guidance on this. > >> > >>> > >> > >>> I've closed the Pull Request, will do appropriate changes and > send in > >> > >>> another for review. > >> > >>> > >> > >>> At Your Service, > >> > >>> Isaac Kamga. > >> > >>> > >> > >>> On Thu, Mar 1, 2018 at 4:56 PM, Myrle Krantz <my...@apache.org> > >> wrote: > >> > >>> > >> > >>>> Hey Isaac, > >> > >>>> > >> > >>>> Replies inline: > >> > >>>> > >> > >>>> On Thu, Mar 1, 2018 at 1:20 PM, Isaac Kamga < > isaac.ka...@mifos.org> > >> > >>>> wrote: > >> > >>>>> I have just updated the copyright information and package name > on > >> the > >> > >>>>> fineract-cn-lang repository and sent in another pull request > >> > >>>>> <https://github.com/apache/fineract-cn-lang/pull/3> for review. > >> > >>>> Please make a pull request with *just* the copyright information > >> > >>>> adjusted. Changing package names is a backwards incompatible > >> change. > >> > >>>> By changing them, you break everything that depends on lang. And > >> all > >> > >>>> of the other fineract cn repositories depend on lang. Package > names > >> > >>>> will have to be changed in all the repositories at once. > >> > >>>> > >> > >>>> We will have to continue to be careful about backwards compatible > >> > >>>> changes until we have three things: > >> > >>>> * signature checking > >> > >>>> * our artifacts in an artifactory > >> > >>>> * an established process of incrementing versions. > >> > >>>> > >> > >>>> Because changing package names is a global change and changing > >> > >>>> copyright information can be done locally, one repository at a > >> time, I > >> > >>>> believe we should adjust the copyright information first. > >> > >>>> > >> > >>>>> On a related note, I'd like to ask experienced developers on > >> Fineract > >> > >>>> CN if > >> > >>>>> it would be necessary to change the project's name ( from > *lang* to > >> > >>>>> *fineract-cn-lang* ) in the settings.gradle > >> > >>>>> <https://github.com/apache/fineract-cn-lang/blob/develop/set > >> > >>>> tings.gradle> > >> > >>>>> file. > >> > >>>> I don't see any reason to change those names. the project's > name is > >> > >>>> appended to the artifact id. If we did this, the artifact id in > >> this > >> > >>>> case would be org.apache.fineract.cn.fineract-cn-lang. It would > >> > >>>> contain duplicated information. Of course we could adjust the > build > >> > >>>> elsewhere to change the artifact id back to lang. But i don't > see a > >> > >>>> benefit in changing the project name. > >> > >>>> > >> > >>>> But perhaps I'm missing something here? > >> > >>>> > >> > >>>>> Also, in the bintray.pkg section of the build.gradle file > >> > >>>>> < > >> > https://github.com/apache/fineract-cn-lang/blob/develop/build.gradle > >, > >> > >>>>> should the `repo` , `userOrg` and `vcsUrl` variables be changed > >> too ? > >> > >>>> These > >> > >>>>> will help with subsequent updates to fineract-cn-api, > >> > >>>>> fineract-cn-cassandra, etc. > >> > >>>> Oops! Good catch. We can delete the bintray section in that > >> > >>>> build.gradlel file until we're ready to introduce the use of the > >> > >>>> apache artifactory. I don't think you'll find a section like > that > >> in > >> > >>>> any other fineract cn project. I was experimenting there. > >> > >>>> > >> > >>>> Thank you for taking this on Isaac, > >> > >>>> Myrle > >> > >>>> > >> > >>> > >> > > >> > > >> >
