2017-02-04 19:19 GMT+01:00 Yasser Zamani <yasser.zam...@live.com>: > 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?
Depends :) If it is possible, smaller steps is my prefered way (resolve one then another issue) but sometimes there is no other option than do it as a huge refactoring. Targeting one issue on PR is the way to go, even if the PR gets large. > 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? It's enough but adding additional tests which cover the new issue is the best approach (not always feasible). It would be nice to have a unit test which fails when issue is not fixed (it can be an existing unit test which was modified to properly test code without issue) > 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? It also depends :) If it is possible to resolve issue without coping, that's good, but if there is already a fix at hand and the solution isn't too specific (which can harm users at some point) I don't mind borrowing code ;-) Regards -- Łukasz + 48 606 323 122 http://www.lenart.org.pl/ --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org For additional commands, e-mail: dev-h...@struts.apache.org
