These all sound good to me Micah, thanks for taking this on! Regarding the javadoc codestyle in (2), I believe it was disabled because there were just too many issues of missing docs at the time. Any documentation additions are definitely welcome and hopefully we can eventually enable the check :)
Thanks, Bryan On Tue, Feb 26, 2019 at 10:17 PM Micah Kornfield <emkornfi...@gmail.com> wrote: > SGTM, I probably start filing JIRAs on Friday if no objections arise. > > On Tue, Feb 26, 2019 at 12:25 AM Praveen Kumar <prav...@dremio.com> wrote: > > > Hi Micah, > > > > Thanks for bringing this up. > > > > I am +1 for all of them. Especially the lack of documentation in the the > > classes is a little hard - i found myself checking documentation of > > corresponding C++ classes to understand format initially. > > > > Lets wait a couple of days for more feedback and create actionable JIRAs > > for the same? > > > > Thx. > > > > On Tue, Feb 26, 2019 at 11:34 AM Micah Kornfield <emkornfi...@gmail.com> > > wrote: > > > >> Hi Java Arrow-Developers, > >> I've been looking more into the java code base and I was wondering if > >> people think any of the following might be worthwhile (or are strictly > >> against them). My java infrastructure knowledge is a little stale, so > if > >> a > >> suggestion I make is absolutely ridiculous I apologize. > >> > >> 1. Upgrade to JUnit 5 (the main advantage I know if is it has much > >> cleaner > >> syntax for asserts with Java8) [1] > >> 2. Try to get a state where we can enforce with Checkstyle that at > least > >> every class has a javadoc comment? > >> 3. Try to get the code to state where it shows no warnings in IntelliJ. > >> At least in the Vector classes there seem to at least be warning about > >> possibly making some methods/classes have more restricted visibility, I > >> don't know if this due to insufficient test coverage or if they are > >> legitimate. If it is the former, is there someplace we can pull in more > >> tests to exercise the code? > >> 4. Setup static type checking assuming NonNull values and annotate > where > >> values can be null (based on limited research [2][3][4] the checker > >> framework might have the right set of annotations to use > >> > >> Thanks, > >> Micah > >> > >> > >> > >> [1] > >> > >> > https://stackoverflow.com/questions/40268446/junit-5-how-to-assert-an-exception-is-thrown > >> [2] https://github.com/google/guava/issues/3031 > >> [3] https://github.com/google/guava/issues/2960 > >> [4] > >> > >> > https://stackoverflow.com/questions/4963300/which-notnull-java-annotation-should-i-use > >> > > >
