Regarding formatting: i’m talking more on these lines.. http://code.revelc.net/formatter-maven-plugin/examples.html#Setting_Source_Files <http://code.revelc.net/formatter-maven-plugin/examples.html#Setting_Source_Files>
If you ok to chat on any IM let me know what do you use. I would connect you there. Regards Ashwin > On Nov 8, 2015, at 2:46 PM, Ashwin Rayaprolu <ashwin.rayapr...@gmail.com> > wrote: > > Hi, > > Even i was not sure why that DerbyTest got changed. > > > I initially checked out from > https://git-wip-us.apache.org/repos/asf/metamodel.git > <https://git-wip-us.apache.org/repos/asf/metamodel.git> and then migrated to > a different repository url cloned from > (https://github.com/apache/metamodel.git > <https://github.com/apache/metamodel.git>). Maybe that was reason i see derby > test got changed.. > Not sure about .patch file > I’ll add unit test and checkin to my version of version control by end of > day( Currently not at home) > Regarding final: Im extremely sorry for that..I was trying to implement some > other logic and removed that at that point.. I’ll change that..Its my bad. > I’ll change it to > // From token can be starting with [ > final String tableNameToken; > final String aliasToken > > Will add all checks.. I just wanted to make sure my changes are welcome and > then proceed on all checks hence didn’t think much on that angle.. I’m > planning to add more Datasource context’s in coming week. Already started > working on that..I really appreciate you reviewing my code.. > Regarding formatting: If you have any code formatter configured for maven > that would probably standardize and remove this issue totally. Apart from > that if you have formatter files for eclipse i’ll import them.. Will go > through all rules and fix them. > > > Regards > Ashwin > >> On Nov 8, 2015, at 2:20 PM, kaspersorensen <g...@git.apache.org >> <mailto:g...@git.apache.org>> wrote: >> >> Github user kaspersorensen commented on the pull request: >> >> https://github.com/apache/metamodel/pull/72#issuecomment-154881570 >> <https://github.com/apache/metamodel/pull/72#issuecomment-154881570> >> >> Hi there, >> >> Thank you very much for the contribution! I think this is a good idea, >> but the PR is a bit questionable in it's current shape, so I hope you would >> be interested in maybe taking these feedback points? >> >> A few strange things in this PR which I think are technicalies/mistakes, >> but please clarify: >> * The DerbyTest file already exists in the master branch. Yet on the >> diff it seems it's been added in full. I can't figure out why that is and if >> something changed? Did you copy it, change something in it or? >> * The .patch file also seems to be included in this Pull Request. That's >> also kinda strange. >> >> So just focusing on the change you've done to FromItemParser (this seems >> to be the real "suggestion"): >> * I would love to see a unittest that uses your new code. We have a lot >> of existing parsing tests in for instance the class QueryParserTest. You can >> easily add one or two more cases of parsing with the square brackets. >> * You've removed the final keyword from "tableNameToken" and >> "aliasToken" and instead initialized them to empty strings. This seems like >> a degradation in code quality to me. >> * There's no check on whether the end-bracket is found or not. I can >> imagine a lot of difficult exception scenarios being thrown there without >> giving the user proper feedback. We should safe-guard such code quite a lot >> IMO. >> * The same holds for validation of the alias that may or may not be >> there. And for the fact that an alias should not have spaces in it. >> * On the code formatting side I have a few remarks: >> * Mixed use of tabs and spaces in indentation. >> * No spaces around "else" token is not how we format code. >> >> >> --- >> If your project is set up for it, you can reply to this email and have your >> reply appear on GitHub as well. If your project does not have this feature >> enabled and wishes so, or if the feature is enabled but not working, please >> contact infrastructure at infrastruct...@apache.org >> <mailto:infrastruct...@apache.org> or file a JIRA ticket >> with INFRA. >> --- >
