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).
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 <[email protected]> wrote:
>
> Github user kaspersorensen commented on the pull request:
>
> 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 [email protected] or file a JIRA ticket
> with INFRA.
> ---
