> On Oct. 13, 2014, 9:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java, > > lines 20-55 > > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line20> > > > > Is this change intentional? > > Veena Basavaraj wrote: > yes it is, it is always hard to do merges with every single import added. > > I would like to understand if this is performance concern/ nitpick or is > there some standards we follow for the import order. I did not see it > documented anywhere, would be nice to have a pointer to the code style > expected. > > Jarek Cecho wrote: > I usually don't like asterisks import as they are masking what is > actually used in the module. In this particular case where it's obviously > that we're trying to get all the various queries, I don't mind as it's just > following the very close relationship between those two classes. I was asking > to understand whether it's intentional change or some automatic IDE change as > there are IDEs that are automatically mergin multiple import statements > together. > > Veena Basavaraj wrote: > as you have very nicely pointed out, every single constant in that file > is used. So might as well say * :)
Yeah, I agree that in this case it make sense to do an exception and use * :) > On Oct. 13, 2014, 9:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java, > > lines 20-56 > > <https://reviews.apache.org/r/26657/diff/1/?file=719689#file719689line20> > > > > Nit: Seems as unnecessary changes? > > Veena Basavaraj wrote: > same as above, is there a import order imposed so I can use that as a > standard in the IDE. If we have more developers contributing to the code > base, projects use a style guide as a standard to avoid debating on such > things:). Time well spent on real issues. > > Jarek Cecho wrote: > I don't think that we have any style guide with regards to the imports > right now. I'm open to include it, but I would be sceptical to apply it > everywhere immediatelly. My concern here will pretty much follow what I've > mentioned elsewhere - this code will make the following commands either > unuseable or troublesome: > > 1) git blame > 2) git cherrypick > > Where both commands are very important in long term project where > multiple developers are working on the code base and trying to figure out > what/when/why. > > Veena Basavaraj wrote: > Dont think we need to apply it everywhere, as new files change we will > apply. > > why is git blame hard if we change and stick to a import order. I am > usign the default order of the IDE right now and this cannot be same for all > IDEs. Git blame will stop at first change, so doing bulk change will pretty much remove make your history unuseable. I'm fine with defining code style and applying it on new files and update the old files as we will refactore those files. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26657/#review56443 ----------------------------------------------------------- On Oct. 13, 2014, 8:03 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26657/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2014, 8:03 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > fix the upgrade logic for 1498 changes. > While fixing the upgrade logic, added comments on how the upgrade logic works. > -renamed test methods to reflect what they are upgrading and what they are > infact testing. > -renamed the internals to exactly mean "schema" upgrades, since we only > allow/ do scheme upgrades the repository > -Also updated the names on the repository api to clearly state the difference > between upgrade and update operations on connector / driver. > > > Diffs > ----- > > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 3ade247 > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > 97de893 > core/src/main/java/org/apache/sqoop/repository/Repository.java 95c7a4d > core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java > c2f8505 > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java > e6e4760 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java > 74e41df > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > 0f0f7c4 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 10a7b1a > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java > cf6e657 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > 56ea147 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > 9316687 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java > fc95222 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestDriverHandling.java > d597bd8 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java > 260c2a9 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInternals.java > 0eb9df4 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > 01a05b2 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java > bbfe5bb > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java > PRE-CREATION > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestSubmissionHandling.java > 8402d8c > > Diff: https://reviews.apache.org/r/26657/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Veena Basavaraj > >
