Making formatting changes is also a refactoring from Git perspective - you don’t have ability to “ignore” those when using git commands.
I do see the point about not caring about ordering imports with git blame as I’m indeed not usually seeing why and when has been given import statement added - I’m looking into the actual code. However I have personally really bad experience when using git cherrypick on files with a lot of import reorders and hence I do feel that the import re-orders belongs to the same category as anything else. If I’m significantly refactoring the file, sure let’s change the order. If I’m changing one line in entire file, then I don’t necessarily see a need to make import re-order change. Jarcec > On Dec 2, 2014, at 11:57 AM, Veena Basavaraj <[email protected]> wrote: > > Not to deviate much from the actual topic and instead focus on resolving > this > > I prefer #1 > > > > > PS: formatting is not refactoring. If a import order changes because we > have standardized on the rules it is really not code refactoring. > If certain methods are renamed, duplicate code collapsed, yes I totally > agree that such changes should be its own patch if possible. > > > > > > > Best, > *./Vee* > > On Tue, Dec 2, 2014 at 10:58 AM, Gwen Shapira <[email protected]> wrote: > >> I believe that the specific case where: >> * patch has very few lines of actual change >> * but has large number of imports >> >> Will be pretty rare and not worth a lot of discussion. I don't think >> its productive to the discussion to focus on what is an edge case. >> Lets try to agree on guidelines that most of us can live with most of the >> time. >> >> In general, most of our patches are fairly large, and I think its a >> good idea to keep our small patches small by not adding much extra >> refactoring. >> >> >> On Tue, Dec 2, 2014 at 8:18 AM, Veena Basavaraj <[email protected]> >> wrote: >>> Let me give a more concrete example >>> >>> #1, if we start using the standards uniformly from now, this means we >> use a >>> IDE formatter that adheres to these rules. >>> >>> So say I am modifying a existing file, I add some 5 new classes and needs >>> imports, I usually tend to use the eclipse shortcut >>> <http://www.rossenstoyanchev.org/write/prog/eclipse/eclipse3.html> >> based on >>> the formatter to do it, it will override and use the new order and there >>> are bound to be changes. So according to you, we just append new imports >>> manually to this file, so we dont create more than 3 lines of changes? >>> >>> I fail to understand why imports re-ordered makes it hard to read a >> change >>> even in git blame. It is very isolated change in the top of the file and >>> can be easily ignored since we know we will be changing it going forward >> to >>> keep one standard. >>> >>> As annoying as it sounds to have this rule, I am happy to document this >>> since everyone contributing to sqoop will be aware of this. >>> >>> >>> >>> >>> >>> Best, >>> *./Vee* >>> >>> On Tue, Dec 2, 2014 at 7:57 AM, Gwen Shapira <[email protected]> >> wrote: >>> >>>> #1 - yes. we had coding standards before, but they are documented now, >>>> which makes for better overall experience for new contributors >>>> >>>> #2 - New code should obviously follow standards. >>>> I support modifying existing code in separate JIRAs when the existing >>>> code is particularly bad and a large change is required to make it >>>> acceptable. >>>> Otherwise, I like Jarcec suggestion: >>>> - Cleanup can be done while working on unrelated patches, provided >>>> that the scope of the cleanup is smaller than that of the patch. I.e. >>>> - if the actual fix is 3 lines, don't add 100 lines of cleanup. If the >>>> actual fix is 100 lines, 3 lines of low-risk cleanup are acceptable. >>>> The idea is that when looking at the JIRA and the patch, 90% of the >>>> patch should be related to the issue described in the JIRA. >>>> >>>> Gwen >>>> >>>> On Mon, Dec 1, 2014 at 7:04 PM, Veena Basavaraj < >> [email protected]> >>>> wrote: >>>>> Very well articulated Gwen. >>>>> >>>>> So the question is >>>>> #1. Should we start using code standards? >>>>> #2, if we do, what is the best way to enforce it? >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> Best, >>>>> *./Vee* >>>>> >>>>> On Mon, Dec 1, 2014 at 6:39 PM, Gwen Shapira <[email protected]> >>>> wrote: >>>>> >>>>>> Regarding applying the changes: >>>>>> >>>>>> I think we want to have good balance between few goals: >>>>>> 1. Minimize risk of changes by having a well defined scope >>>>>> 2. Make reviewing easier >>>>>> 3. Ease of cherry-picking >>>>>> 4. Ease of incrementally improving code quality >>>>>> >>>>>> Our current process leans toward the first three goals, but >>>>>> discourages small improvements by adding quite a bit of process >> around >>>>>> them. >>>>>> >>>>>> I'd like to see us a bit more open toward small code improvements >> that >>>>>> do not add significant risk to the project. Spelling of variable >>>>>> names, import order, indentation, comment readability, etc. I believe >>>>>> it will lead to better code in the long term, without significant >>>>>> drawbacks. >>>>>> >>>>>> Gwen >>>>>> >>>>>> >>>>>> On Mon, Dec 1, 2014 at 6:17 PM, Jarek Jarcec Cecho < >> [email protected]> >>>>>> wrote: >>>>>>> Thank you for putting the coding guidelines together Veena. I’ve >> read >>>>>> the page and I don’t see anything concerning. I have to admit that I >> did >>>>>> not read the whole Google guideline so I’m wondering if there are any >>>> huge >>>>>> differences between current (albeit unwritten) rules and the proposed >>>>>> Google guidelines? >>>>>>> >>>>>>> I’ve added/edited the content a bit, trying to put some small >> comments >>>>>> without changing the semantics of the page. I have following larger >>>> points: >>>>>>> >>>>>>> 1) Line size >>>>>>> >>>>>>> The “General Rules” section specifies that the “Column limit can >> be 80 >>>>>> or 100 characters”, the subsection “ Column Limit and Line Wrapping” >> is >>>>>> stating only “100” and then suggesting that we could also drop any >> line >>>>>> limits. I would suggest to synchronize those places together as it >> seems >>>>>> quite confusing. >>>>>>> >>>>>>> I would personally vote to not have any sharp limit for maximum >> line >>>>>> size. We have 80 characters limit in Sqoop 1 and it makes code very >>>> hardly >>>>>> readable at some places, so I like the Kafka wording “no maximum >> size, >>>> but >>>>>> be reasonable”. >>>>>>> >>>>>>> 2) Applying the changes >>>>>>> >>>>>>> Considering that we might introduce some changes against current >>>>>> (unwritten) rules, I think that we should talk about how we’re going >> to >>>>>> enforce the coding guidelines going forward. This is not a question >> for >>>> new >>>>>> files as those should obviously follow them. I’m more interested to >>>> discuss >>>>>> how we’re going to handle changes in current files. As we are trying >> to >>>>>> keep each patch focusing on given functionality and we’re >> discouraging >>>>>> unrelated changes, I would propose to discourage people to enforce >> those >>>>>> rules in existing files if that would mean to significantly refactor >> the >>>>>> file just for the purpose of being compliant with our code >> guidelines. >>>>>>> >>>>>>> This of course doesn’t mean that people should not follow the >>>> guidelines >>>>>> at all (new content should follow them) or that we’re going to stick >> to >>>> the >>>>>> old formatting forever (on large file refactoring it would be OK to >>>> apply >>>>>> such changes). I’m just worried about introducing a lot of >> unnecessary >>>>>> changes just to keep in sync with coding guidelines. >>>>>>> >>>>>>> Jarcec >>>>>>> >>>>>>>> On Nov 30, 2014, at 6:29 PM, Veena Basavaraj < >>>> [email protected]> >>>>>> wrote: >>>>>>>> >>>>>>>> Qian, >>>>>>>> >>>>>>>> I think we both are on the same page. >>>>>>>> >>>>>>>> I just wrote the guidelines wiki to to come up with the code >> style. >>>>>>>> >>>>>>>> One we all vote on the wiki, we can formulate those rules as IDE >>>>>> specific >>>>>>>> files, like the one you attached for intelliJ >>>>>>>> >>>>>>>> Hope it clears the confusion now. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Best, >>>>>>>> *./Vee* >>>>>>>> >>>>>>>> On Sun, Nov 30, 2014 at 6:20 PM, Xu, Qian A <[email protected]> >>>>>> wrote: >>>>>>>> >>>>>>>>> Hi Veena, >>>>>>>>> >>>>>>>>> A code guideline will definitively help both developers and >> reviews >>>> to >>>>>>>>> contribute code better. I'd suggest apply code style to new code >>>>>>>>> contribution first. >>>>>>>>> >>>>>>>>> --Qian (Stanley) Xu >>>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: Veena Basavaraj [mailto:[email protected]] >>>>>>>>> Sent: Monday, December 01, 2014 10:09 AM >>>>>>>>> To: [email protected] >>>>>>>>> Subject: Re: Sqoop 2 JAVA coding guidelines >>>>>>>>> >>>>>>>>> Thanks Qian, I will attach it, do you have any comments on the >>>>>> guidelines? >>>>>>>>> We should all vote for the wiki guidelines before we attach the >>>>>> formatter. >>>>>>>>> >>>>>>>>> Please send an email to the dev@ list asking permission to edit >> the >>>>>> wiki. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> *./Vee* >>>>>>>>> >>>>>>>>> On Sun, Nov 30, 2014 at 5:59 PM, Xu, Qian A <[email protected] >>> >>>>>> wrote: >>>>>>>>> >>>>>>>>>> Hi there. >>>>>>>>>> >>>>>>>>>> I don’t have the privilege to add attachment to Wiki , so I've >>>>>>>>>> attached code formatter of IntelliJ (v13.1). I hope this helps. >>>>>>>>>> >>>>>>>>>> --Qian (Stanley) Xu >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Gwen Shapira [mailto:[email protected]] >>>>>>>>>> Sent: Monday, December 01, 2014 8:46 AM >>>>>>>>>> To: [email protected] >>>>>>>>>> Subject: Re: Sqoop 2 JAVA coding guidelines >>>>>>>>>> >>>>>>>>>> I think its a great idea to document the coding standards that >>>> already >>>>>>>>>> exist for this project. >>>>>>>>>> It can be frustrating to new contributors to have to guess their >>>> way >>>>>>>>>> around our un-written expectations. >>>>>>>>>> >>>>>>>>>> Gwen >>>>>>>>>> >>>>>>>>>> On Sun, Nov 30, 2014 at 2:05 PM, Veena Basavaraj >>>>>>>>>> <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>>> Hey all, >>>>>>>>>>> >>>>>>>>>>> Since a few months I have been contributing to Sqoop2, I do >>>> notice a >>>>>>>>>>> lack of style guide for sqoop2 code. >>>>>>>>>>> >>>>>>>>>>> I have started a wiki highlighting things for developers to >>>> follow. >>>>>>>>>>> >>>>>>>>>>> >>>> https://cwiki.apache.org/confluence/display/SQOOP/Sqoop+2+Coding+Gui >>>>>>>>>>> de lines I prefer the google java style guide to begin with. >>>>>>>>>>> >>>>>>>>>>> Please add your comments in the comments section on things you >>>> would >>>>>>>>>>> like to have, so we can iterate over this in the coming few >> weeks >>>>>>>>>>> and settle down to a standard that every one can use in their >>>> IDE. I >>>>>>>>>>> am happy to create a formatter XML for eclipse as per these >> rules >>>>>>>>>>> and attach it to the wiki. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Best, >>>>>>>>>>> *./Vee* >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>>> >>>> >>
