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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
