You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@tinkerpop.apache.org by leventov <gi...@git.apache.org> on 2018/03/17 16:01:25 UTC

[GitHub] tinkerpop pull request #819: Use error-prone compiler

GitHub user leventov opened a pull request:

    https://github.com/apache/tinkerpop/pull/819

    Use error-prone compiler

    See http://errorprone.info/. Fixes a number of bugs.
    
    This is just first step; maintainers of this project are encouraged to enable more checks eventually, e. g. "Experimental ERROR" patterns from here: http://errorprone.info/bugpatterns

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/leventov/tinkerpop error-prone

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/tinkerpop/pull/819.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #819
    
----
commit 32ea0b1192b3cd2dfa8aa12fc0e55ef2ec48ccb1
Author: leventov <le...@...>
Date:   2018-03-17T15:56:23Z

    Use error-prone compiler

----


---

[GitHub] tinkerpop issue #819: Use error-prone compiler

Posted by spmallette <gi...@git.apache.org>.

Github user spmallette commented on the issue:

    https://github.com/apache/tinkerpop/pull/819
  
    Thanks for taking a look at our build. Unfortunately, this kind of change can't really be considered as part of just a pull requeswt. we'd need significant community discussion about (1) whether or not we want our build altered with more tooling at this stage of its development (2) if we did want more tooling, we'd need to discuss the right tool for the job (it could be "errorprone", it might not be - i'm sure people have lots of opinion on this topic) (3) if we did actually choose one or more, then we'd need to decide which checks we would enable. Anyway, if you'd like to lead that discussion to get a change like this in, please begin a DISCUSS thread on our dev mailing list. Thanks again.


---

[GitHub] tinkerpop issue #819: Use error-prone compiler

Posted by spmallette <gi...@git.apache.org>.

Github user spmallette commented on the issue:

https://github.com/apache/tinkerpop/pull/819

sorry - i didn't see your reply earlier i the week. i think this PR even without the build changes had problems. the PR branch wasn't building I don't think (i seem to remember @robertdale bringing that up at various points). was all that fixed?

thus far we've only talked about the build changes as I thought that was the primary point of the PR so i never bothered to comment much on the changes themselves. since you ask about that now, i'd say that they are hard to evaluate as they have been submitted. Because it is one giant PR, I don't understand the reasoning for these changes. You do have some detailed comments about the changes to `TraversalStrategy` which is nice, but that body of change is terrifying to be honest. It wouldn't surprise me if that was the source of the build problems. That sort of change to that important area of code would require its own pull request and could require a discussion on the dev list.

So, reopen this PR? - I'd say "no". I'd say that if you feel really strongly about these changes for some reason, then they should be broken apart into new individual PRs so that they can each be evaluated on their own merit. As a reviewer (and you need 3 committers to vote +1 for us to merge), personally I'd be much happier to see PRs that do more than add generics or reformat imports because some tool said to do it. Like, if you truly have a much better way to sort traversal strategies - wow...that would be awesome! Or if traversal strategy application was bug prone and that tool your using uncovered that and it fixed a subtle problem no one ever noticed - excellent! But, please frame your PR in that fashion so that we understand what we're exactly evaluating. Organizing your work that way will help immensely on our side.

Hope that helps frame my opinion on this. I'm sure someone else will chime in if they think something different. Thanks!

---