Thank you so much for useful comments. I also wait for Lukasz, Aleksandr and others for any comments and then will update my PR to satisfy that it's a helpful PR as much as possible.
I also discovered sometimes PRs will be dependent e.g. PR2 will fix ISSUE2 if and only if PR1 has been accepted and pulled before. I think I should fix ISSUE2 in PR1 too and links them in JIRA, all of these before PR1's acceptation, to avoid make multiple dependent PRs with shared changes, right? On 2/6/2017 2:22 PM, Christoph Nenning wrote: > Hi, > > in general I prefer to have better and bigger PRs :) > > >> 1. In a PR, Do you recommend to add an unit test which tests that >> specific issue or similar issues? Or no, make current unit tests passing > >> is enough and recommended? > > > I love having more tests ;) > Tests for specific issues are alright. > > > >> 2. When I fixed WW-4694, I discovered that our AnnotationUtils has not >> any tangible improvement for about 4 years and also, with my changes, a >> method named isAnnotatedBy will be almost useless or duplicate. But >> Spring has a similar class with same name and purpose which has been >> wrote more robust and much newer than our one. I'm sure I can merge them > >> carefully with respect to not breaking the rest of the current codes and > >> also, merge only until my changes get clear, beauty and known future >> bugs free, not merge everything. But is it recommended by you? Or no, >> you prefer fewer changes but with higher possibility of future bugs and >> maybe ugly code as changes made day by day? > > > Copying code from other projects my cause legal issues. You should avoid > doing that. But it would be great to refactor and improve AnnotationUtils. > If old methods can be removed it is great, too. > > > > Regards, > Christoph > > > >> From: Yasser Zamani <yasser.zam...@live.com> >> To: Struts Developers List <dev@struts.apache.org>, >> Date: 04.02.2017 19:19 >> Subject: Re: How to select how to solve issue? >> >> I have fixed WW-4694 where I found that I need some recommendation >> before creating it's PR, to fix it as best as possible. >> >> In general, would you like to issue being solved with fewer changes as >> much as possible? Or no, as best as possible but with some more changes? >> >> In more specific: >> >> 1. In a PR, Do you recommend to add an unit test which tests that >> specific issue or similar issues? Or no, make current unit tests passing > >> is enough and recommended? >> >> 2. When I fixed WW-4694, I discovered that our AnnotationUtils has not >> any tangible improvement for about 4 years and also, with my changes, a >> method named isAnnotatedBy will be almost useless or duplicate. But >> Spring has a similar class with same name and purpose which has been >> wrote more robust and much newer than our one. I'm sure I can merge them > >> carefully with respect to not breaking the rest of the current codes and > >> also, merge only until my changes get clear, beauty and known future >> bugs free, not merge everything. But is it recommended by you? Or no, >> you prefer fewer changes but with higher possibility of future bugs and >> maybe ugly code as changes made day by day? >> >> Thanks in advance for your reading! >> >> On 2/3/2017 2:23 AM, Yasser Zamani wrote: >>> Thank you Aleksandr, I made my first one >>> at https://github.com/apache/struts/pull/116 😉 >>> >>> >>> Thank you Lukasz for your explanation. >>> >>> >>> Thanks all. >>> >>> >>> Regards, >>> >>> Yasser. >>> >>> >>> >>> > ------------------------------------------------------------------------ >>> *From:* Aleksandr Mashchenko <amashche...@apache.org> >>> *Sent:* Wednesday, February 1, 2017 8:40 PM >>> *To:* dev@struts.apache.org >>> *Subject:* Re: How to select which issue to work on? >>> >>> Hi, >>> >>> > does 'unassigned' mean no one already work on it? >>> Usually, yes. >>> >>> > is it better to solve old reported one or newer ones? >>> Pick the one you can solve. Just make sure that particular issue is >>> still relevant. >>> >>> Looking forward to see PR-s from you. >>> >>> --- >>> Regards, >>> Aleksandr >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org >>> For additional commands, e-mail: dev-h...@struts.apache.org >>> >> >> --- >> This email has been checked for viruses by Avast antivirus software. >> https://www.avast.com/antivirus >> > > > This Email was scanned by Sophos Anti Virus > --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org For additional commands, e-mail: dev-h...@struts.apache.org
