[GitHub] metamodel issue #153: Collections instead of Arrays

Github user kaspersorensen commented on the issue: https://github.com/apache/metamodel/pull/153 I'll go ahead and merge this, good work. Thank you for the considerable contribution. --- If your project is set up for it, you can reply to this email and have your reply appear on

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user kaspersorensen commented on the issue: https://github.com/apache/metamodel/pull/153 +1 --- 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

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user kaspersorensen commented on the issue: https://github.com/apache/metamodel/pull/153 I've only got more compiler warnings that I'd like to fix before I give this my +1. I've submitted those in a PR for you: https://github.com/tomatophantastico/metamodel/pull/1 --- If

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user kaspersorensen commented on the issue: https://github.com/apache/metamodel/pull/153 Can you open a PR for the checkstyle improvement then also? :-) Btw. the 5.x branch is no longer in use really. We've merged it into master as of #147. I will take a look

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user tomatophantastico commented on the issue: https://github.com/apache/metamodel/pull/153 I manually rearranged the imports. To make things easier improve quality i also created a checkstyle integration in

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user kaspersorensen commented on the issue: https://github.com/apache/metamodel/pull/153 Other than the compiler warnings, the code looks good to me. --- 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

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user kaspersorensen commented on the issue: https://github.com/apache/metamodel/pull/153 I see a lot of unused imports on this branch:  --- If your project is set

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user kaspersorensen commented on the issue: https://github.com/apache/metamodel/pull/153 Great. That makes sense. I expect to take a closer look at the diff sometime this weekend. --- If your project is set up for it, you can reply to this email and have your reply appear on

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user tomatophantastico commented on the issue: https://github.com/apache/metamodel/pull/153 IIRC, guava is only in the test cases, so i would just set the scope to test --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user kaspersorensen commented on the issue: https://github.com/apache/metamodel/pull/153 I also wanna applaude this big effort :-) But like @LosD I am a bit concerned with adding Guava to the core module because that means our API and really core classes essentially depends on

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user tomatophantastico commented on the issue: https://github.com/apache/metamodel/pull/153 Sorry, this was more of the reasoning behind my bulk changes & the changes in the (im)mutable table class, the implementor should of course pick what fits the scenario best. --- If

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user LosD commented on the issue: https://github.com/apache/metamodel/pull/153 I don't really agree on "ArrayLists should be used", especially for single-element lists, as a singletonList does not need a backing array, but it's not really a blocker for me. --- If your

[GitHub] metamodel issue #153: Collections instead of Arrays

Github user tomatophantastico commented on the issue: https://github.com/apache/metamodel/pull/153 w.r.t. guavas Lists.newArrayList() I think i used it mainly in the testcases, purely for syntactic convenience. Internally ArrayLists should be used as it allows for more