> On Nov. 29, 2014, 9:57 p.m., Jarek Cecho wrote: > > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java, > > lines 21-52 > > <https://reviews.apache.org/r/28537/diff/1/?file=778388#file778388line21> > > > > Can we please not rearange imports as part of this patch? > > Veena Basavaraj wrote: > Let me highlight one thing. > > there needs to be a standard we follow in all files. > > > > In the CSVIntermediateDataFormat. the order is: > > com > org > java > javax > I put this in the "IDE" fomratter. > > the test file has some other random order. we cannot have this, it is > super in productive, if you want this to continue, lets stop been anal about > such a trivial thing. > > My 2 cents, it would be good for the project to have a standardized order > when I import new things, I use a shortcut in IDE and let the formatter take > care of it. > > I dont manually type in every new import, that is not my good use of time. > > Jarek Cecho wrote: > You're correct that we are missing proper guidelines on how the import > should be ordered and I'm all for to define that. We should however have a > discussion about that in separate JIRA, not part of a different unrelated > JIRA. > > I'm wondering how is this making you a less productive? I'm not typing > each import manually myself, I'm using my IDE (InteliJ) to do that for me. > All I'm asking to not rearange the imports automatically. > > Veena Basavaraj wrote: > If you read my comment above carefully, I have even told you to the order > I use in the IDE, based on the CSVIntermediateDataFormat class, and this > thing seems to be custom order in each file. > > What is your issue with sticking to one order that is in > CSVIntermediateDataFormat > > com > org > java > javax > > Really you think spending time on a RB on import prder is the best use of > your time and my time? An IDE has one set of rules. When I do CLTR+SHIFT+O, I > have not seen an IDE that has rules per file yet, > > Jarek Cecho wrote: > I don't have a problem with defining import order in the project. I > however believe that such change should be discussed openly in standalone > JIRA rather then on Review board as a part of unrelated change. > > I'm not familiar with CTLR+SHIFT+O shortcut. Doing little bit of googling > it seems that it's an Eclipse specific shortcut to re-organize imports. Let > me know if I'm incorrect here. If that is right, then until we will have set > of rules to follow, it seems to me that we can simply not call this shortcut? > If all that the shortcut does is to reorganize the imports, then I'm assuming > it won't have any impact on productivity? > > The reason why we are trying not to have unrelated changes in patches > (such as re-organizing imports, ...) is that doing unrelated changes makes > difficult to use commands such as "git blame" and "git cherrypick" that we > are using very often. > > Veena Basavaraj wrote: > not calling the shortcut means I manually type in every import. > > It adds the imports that are missing as well. > > > kafka project for instance does not really worry about the import order. > > I have spent time to create a wiki on style guide and sent the email to > dev@. High time we had this. I vote for google style guide that is standard > across many projects. If you have others in mind, leave a feedback there. > > If we enforce a standard and there is a import re-ordering this means > that it is part of the change and doing a git blame should be > self-explanatory. > > Veena Basavaraj wrote: > As far as this patch is concerned, I am ok to put this on hold until > there is resolution. > > If everyone in the sqoop dev list votes for not having a standard and > then assumes that we will keep a random order in every file just to avoid any > more changes, I want to see this discussion in the dev@ list. > > Jarek Cecho wrote: > Thank you for putting together the code guidelines. I'll definitely read > them and follow up there. > > I don't se a need to hold this patch though. Can we simply drop the > re-arrangement and commit the patch? > > Veena Basavaraj wrote: > for this patch I will, but any patch going forward, it is best we have a > resolution. > > lets take a vote on the following, I will send an email to dev@ > > 1. We do not care about the order, so lets not bother if reorder occurs, > there are projects that follow this ( kafka) > 2. We do care, lets impose the rule going forward on any file we touch > 3. We do care a lot, lets fix every file > 4. We do care, but lets not modify any existing file, but only for new > files, if a developer by mistake does it, lets not frown upon it, just point > them to the wiki.
Thank you for updating the patch. I'll run tests and commit if everything passes. Let's take the formating discussion to the email thread at dev@, so that everyone have ability to jump in. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28537/#review63297 ----------------------------------------------------------- On Dec. 1, 2014, 8:05 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28537/ > ----------------------------------------------------------- > > (Updated Dec. 1, 2014, 8:05 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1750 > https://issues.apache.org/jira/browse/SQOOP-1750 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see jira > > > Diffs > ----- > > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java > 4f2baf9 > > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java > b629897 > > Diff: https://reviews.apache.org/r/28537/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Veena Basavaraj > >
