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!
---
