I totally agree. Robert's statement "Any person can choose to run any
preferred tool on a local copy of the source code, ..." nailed it. I do
that from time to time, and it's nice to end up with clean static code
analysis reports, but I don't think it's something we should enforce. When
I commit a file, I only make sure that IntelliJ doesn't show any warnings;
in my opinion, that's a good enough baseline.

Cheers,
Daniel


On Mon, Apr 9, 2018 at 7:27 AM, Stephen Mallette <spmalle...@gmail.com>
wrote:

> I'm not sure if there's any additional discussion coming on this thread,
> but I still feel pretty strongly that (1) "more tools is not better" and
> (2) we are basically at a tool selection phase with leanings toward one
> with multi-language support (which errorprone is not). I agree with
> Robert's assertion that folks can use whatever tools they want locally
> however, I don't think we want to encourage a mixed bag of "code cleanup"
> type pull requests either. If there is something important that needs fixup
> it should be isolated and likely presented here for discussion. Any other
> thoughts/opinions?
>
>
>
> On Tue, Apr 3, 2018 at 2:08 PM, Robert Dale <robd...@gmail.com> wrote:
>
> > It's definitely hard enough to get one tool done right, as demonstrated
> by
> > the broken PR, let alone trying to get several tools done right.  It
> makes
> > sense for our team to choose one tool that's most broadly applicable. We
> > can master that one tool and use it to its full extent. Then we can
> > evaluate what's not covered and decide on the right tool for that job.
> So,
> > right, we as a project need to decide on what we're willing and able to
> > support. However, this process does not impose any limitation on what an
> > individual can do. Any person can choose to run any preferred tool on a
> > local copy of the source code, report findings, fix, etc based on that
> > tool.  In that case, the more the merrier!
> >
> > The error-prone PR could be saved if it removed the error-prone compiler,
> > made some statements on why certain changes were made, and built cleanly.
> >
> > Robert Dale
> >
> > On Tue, Apr 3, 2018 at 12:57 PM, Stephen Mallette <spmalle...@gmail.com>
> > wrote:
> >
> > > Sure, I think that I was the one to specifically say that I didn't
> agree
> > > that "running more tools was better". Sorry, I should have said more on
> > > that to explain why I felt that way.
> > >
> > > Shouldn't projects consider the cost to each new tool that gets added
> the
> > > build chain? It seems undesirable to me for a project to run findbugs
> and
> > > coverity and errorprone and whatever else just because there is some
> > chance
> > > that one might find something the other doesn't at the additional cost
> to
> > > build time (i don't what that might be - maybe not an issue?) and
> ongoing
> > > maintenance. In my mind there should be some added value for each new
> > thing
> > > we add to the build chain. I don't know that the added value needs to
> be
> > > significant (that would be nice), but it should at least be explainable
> > in
> > > a definitive way (e.g. "we need to add findbugs because it covers X,
> Y, Z
> > > and errorprone does not").  I think that I'd want that kind of
> > explanation
> > > if someone moved to add a second tool after we'd chosen a first before
> I
> > > would jump on-board with such a change.
> > >
> > > To continue with the issue of tool selection while I think I can agree
> > that
> > > one tool may or may not find "everything", right now, we have no tools
> in
> > > place for this at all. It is a green field. If we should introduce a
> tool
> > > it should be one that gets us the most bang for the buck and that would
> > be
> > > one that handles multiple languages. We would focus on getting that
> > > particular tool into play first and then go through the process of
> > turning
> > > on the various checks we want in that tool and, as we have multiple
> > > languages, we'd want to do that for each language we support. Once all
> > that
> > > is done, then we would determine if there are things that aren't
> covered
> > > and whether or not additional tools are helpful or redundant.
> > >
> > > Adding stuff (i.e. "tools" but also features or new dependencies) just
> > > because it is available or because it provides a small incremental
> value
> > > usually doesn't feel right to me. We've haphazardly done things like
> this
> > > in the past and it's always ended up burning us which is why I'm
> > advocating
> > > for a slow and logical approach to making this happen. For example,
> > > sometime back, we added an appveyor build to try to ensure the project
> > > built on windows. But after a while we came to realize that it was
> false
> > > failing on a fairly regular basis. The failed builds basically put red
> > "X"s
> > > on all the PRs even though they were fine which made it hard to quickly
> > > figure out what was truly passing and what wasn't. We were dealing with
> > all
> > > this extra effort just so that if someone on windows pulled the code
> they
> > > could build TinkerPop if they wanted. We learned that having appveyor
> in
> > > the mix was a small incremental value that wasn't really worth the
> effort
> > > expended since we generally found that no one was really building on
> > > windows in the first place. I can't even recall the last complaint
> about
> > > that issue.
> > >
> > > Anyway, I hope that explains where I'm coming from a bit.
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > On Tue, Apr 3, 2018 at 11:24 AM, Roman Leventov <leventov...@gmail.com
> >
> > > wrote:
> > >
> > > > Could somebody explain how adding a multi-language tool makes adding
> a
> > > > single-language tool undesirable, given one doesn't strictly cover
> > > another?
> > > >
> > > > On Tue, 3 Apr 2018, 16:36 Stephen Mallette, <spmalle...@gmail.com>
> > > wrote:
> > > >
> > > > > Reviving this thread a bit - a quick summary of where things were
> > left:
> > > > >
> > > > > 1. no one seems specifically against adding a tool of this kind
> > (though
> > > > > Florian went so far as to be positive on doing so)
> > > > > 2. if we did add a tool it was a good suggestion to get the tool
> > > selected
> > > > > first and then slowly enable different checks.
> > > > > 3. having multi-language support in the tool was brought up as a
> > > possible
> > > > > requirement in tool selection
> > > > >
> > > > > the more i've thought about it the more that I think my preference
> > for
> > > > > putting code analysis in place would involve using a tool that
> > provided
> > > > > multi-language support. Therefore, something like sonarqube or
> > coverity
> > > > > would be more appealing to me than the original JVM only solution
> > that
> > > > was
> > > > > proposed. Is that basically the representative feeling on this for
> > > > > everyone?
> > > > >
> > > > >
> > > > >
> > > > > On Wed, Mar 21, 2018 at 10:03 AM, Robert Dale <robd...@gmail.com>
> > > wrote:
> > > > >
> > > > > > It has multi-language support -
> > > > > > https://community.synopsys.com/s/question/
> 0D53400003WcsSiCAJ/what-
> > > > > > languages-are-supported-by-synopsys-static-analysis-coverity
> > > > > >
> > > > > > Robert Dale
> > > > > >
> > > > > > On Wed, Mar 21, 2018 at 10:01 AM, Robert Dale <robd...@gmail.com
> >
> > > > wrote:
> > > > > >
> > > > > > > Another option might be to use Coverity -
> > > > > https://scan.coverity.com/faq
> > > > > > -
> > > > > > > and run it in Travis CI
> > > > > > >
> > > > > > > Robert Dale
> > > > > > >
> > > > > > > On Wed, Mar 21, 2018 at 9:43 AM, Stephen Mallette <
> > > > > spmalle...@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > >> >  I think any proposed changes would be separate PRs and
> > reviewed
> > > > on
> > > > > a
> > > > > > >> case-by-case
> > > > > > >> basis.
> > > > > > >>
> > > > > > >> yeah - i think if we want something like this the goal should
> > > first
> > > > be
> > > > > > to
> > > > > > >> decide on a tool (i can't say that i agree with Roman that
> > > "running
> > > > > more
> > > > > > >> tools is always better" btw). note i'm still saying, "if we
> want
> > > > > > something
> > > > > > >> like this" because while i'm not against it I'd like to be
> > > convinced
> > > > > > that
> > > > > > >> we need to worry about this now at this stage of TinkerPop's
> > > > > > development.
> > > > > > >> i'd have been more inclined to deal with this early on in
> > > > TinkerPop's
> > > > > > >> development rather than now. And, yes, also agree that i would
> > > like
> > > > to
> > > > > > see
> > > > > > >> the tool in first and then each introduction of "rule" be a
> > > separate
> > > > > > >> discussion and PR.
> > > > > > >>
> > > > > > >> > As for making it the default compiler, I think this would
> be a
> > > > > > profile,
> > > > > > >> disabled by default, at best at least until a time it can be
> > > > massaged
> > > > > to
> > > > > > >> where it works for this project.
> > > > > > >>
> > > > > > >> yeah....that might be a smart way to go
> > > > > > >>
> > > > > > >> >  whether other static analysis tools would find most of the
> > > > problems
> > > > > > >> that
> > > > > > >> such a compiler can find and also work with other languages
> than
> > > > Java.
> > > > > > >>
> > > > > > >> very good thought. while the bulk of our code is java, we are
> > far
> > > > > from a
> > > > > > >> single language code base. if there is one tool that can give
> us
> > > > some
> > > > > > nice
> > > > > > >> clean analysis across the entire code base, perhaps that's the
> > > best
> > > > > way
> > > > > > to
> > > > > > >> go (again, "if we want something like this").
> > > > > > >>
> > > > > > >>
> > > > > > >>
> > > > > > >>
> > > > > > >> On Tue, Mar 20, 2018 at 8:41 AM, Florian Hockmann <
> > > > > > f...@florian-hockmann.de
> > > > > > >> >
> > > > > > >> wrote:
> > > > > > >>
> > > > > > >> > In general, I am +1 on adding a static analysis tool to our
> > tool
> > > > > > chain.
> > > > > > >> > I just wonder whether a Java compiler is the best choice for
> > us
> > > > > right
> > > > > > >> > now or whether other static analysis tools would find most
> of
> > > the
> > > > > > >> > problems that such a compiler can find and also work with
> > other
> > > > > > >> > languages than Java. SonarQube[1] for example looks pretty
> > nice.
> > > > It
> > > > > > also
> > > > > > >> > supports Python, C#, and JavaScript and we can probably just
> > > > > integrate
> > > > > > >> > it via GitHub to generate reports for the release branches
> and
> > > all
> > > > > PRs
> > > > > > >> > where it would then add issues in the form of inline
> comments.
> > > > > > >> >
> > > > > > >> > [1] https://www.sonarqube.org/
> > > > > > >> >
> > > > > > >> > Am 20.03.2018 um 03:42 schrieb Robert Dale:
> > > > > > >> > > Some changes look good others look wrong evident by the
> > failed
> > > > > > >> builds.  I
> > > > > > >> > > think any proposed changes would be separate PRs and
> > reviewed
> > > > on a
> > > > > > >> > > case-by-case basis.  As for making it the default
> compiler,
> > I
> > > > > think
> > > > > > >> this
> > > > > > >> > > would be a profile, disabled by default, at best at least
> > > until
> > > > a
> > > > > > >> time it
> > > > > > >> > > can be massaged to where it works for this project.
> > > > > > >> > >
> > > > > > >> > > On Mon, Mar 19, 2018 at 14:59 Roman Leventov <
> > > > > leventov...@gmail.com
> > > > > > >
> > > > > > >> > wrote:
> > > > > > >> > >
> > > > > > >> > >> See https://github.com/apache/tinkerpop/pull/819.
> > > > > > >> > >>
> > > > > > >> > >> 1) Are there any objections to using error-prone compiler
> > > > instead
> > > > > > of
> > > > > > >> > >> OpenJDK's javac compiler?
> > > > > > >> > >> 2) Stephen raised the question, that Tinkerpop project
> may
> > > > better
> > > > > > use
> > > > > > >> > >> another static analysis tool instead of error-prone. I
> have
> > > to
> > > > > > answer
> > > > > > >> > that
> > > > > > >> > >> no static analysis tool covers 100% of the errors found
> by
> > > any
> > > > > > other
> > > > > > >> > tool,
> > > > > > >> > >> so running more tools is always better.
> > > > > > >> > >> 3) Stephen raised the question, what checks should be
> > > enabled.
> > > > I
> > > > > > >> believe
> > > > > > >> > >> this is out of scope of the PR (
> > > > > > >> > >> https://github.com/apache/tinkerpop/pull/819),
> > > > > > >> > >> because it doesn't (and doesn't intent to) enable  any
> more
> > > > > checks
> > > > > > >> > beyond
> > > > > > >> > >> the default (= minimal) set of error patterns. More
> checks
> > > may
> > > > or
> > > > > > may
> > > > > > >> > not
> > > > > > >> > >> be enabled in later PRs and that could be discussed
> > > separately.
> > > > > > >> > >>
> > > > > > >> >
> > > > > > >> >
> > > > > > >> >
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Reply via email to