David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/11235 )
Change subject: IMPALA-7399: Remove third-party dependencies from junit xml script ...................................................................... Patch Set 4: (1 comment) > I don't feel all that strongly about this stuff, but let me know what you > think. Sure, happy to discuss. To me one, long function seems like it would be annoying to read. I actually started to write this as a series of functions, but quickly switched to a class for a couple of (probably minor) reasons. - Multiple functions took the same params, just with subtle variation, e.g., the immediate parent of the child that was being created, phase, step, and sometimes time or timestamp. It made main() and the function signatures repetitive and messy. The class seemed like a cleaner approach, and it felt like the amount of code decreased. I thought it was more readable. - I tend to write my classes in a similar style. Member variables that are just scalars get assigned immediately, and anything that requires some computation gets a @property, though I agree there's not a lot to be saved in terms of lazy loading here. But it feels like enough of a language convention that I don't really think about it. - When I find myself calling the same function four or five times in immediate succession, I usually default to a loop, instead of writing root.set(a, b), root.set(c, d), root.set(x, y), etc. I don't mind having essentially a temp dictionary. As you said, it's a matter of style. I can change it. http://gerrit.cloudera.org:8080/#/c/11235/4/bin/rat_exclude_files.txt File bin/rat_exclude_files.txt: http://gerrit.cloudera.org:8080/#/c/11235/4/bin/rat_exclude_files.txt@13 PS4, Line 13: bin/generate_junitxml.py > Does rat not ignore symlinks by default? Sadly, no. https://jenkins.impala.io/job/gerrit-code-review-checks/353/ 19:39:45 ] UNAPPROVED: bin/generate_junitxml.py -- To view, visit http://gerrit.cloudera.org:8080/11235 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I958ee0d8420b6a4197aaf0a7e0538a566332ea97 Gerrit-Change-Number: 11235 Gerrit-PatchSet: 4 Gerrit-Owner: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Nithya Janarthanan <njanartha...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Wed, 15 Aug 2018 23:52:28 +0000 Gerrit-HasComments: Yes
