wartremover?

[eparejatobes]

I know this is planned #16, but I need this ASAP. We should try to get from that list the 2 or 3 critical things that we need like now and do a small release. @laughedelic ?

[laughedelic]

uff.. I'll try to do this and https://github.com/era7bio/scala-2.10.g8/issues/9 by the end of the week.

[eparejatobes]

Adding wartremover is pretty easy. What do you see as essential from #16 ?

[laughedelic]

• update literator and taglist plugins (easy)
• fix: error messages instead of exceptions
• fix: pushApiDocsToGHPagesAction check vcs operations like remote fetching

I mean just that I have a lot of other stuff to do.. like scarph and the intercrossing report..

[laughedelic]

hi @eparejatobes.

I've added wartremover in #25, you can try 0.5.0-SNAPSHOT. There is some issue though: I can't release it now, because wartremover fails on the plugins' source... "/

[eparejatobes]

ok fine

[laughedelic]

ok, I returned to this thing. I suggest to determine the list of warts that we want to use, because some of them work not so well, some are just not that useful. Here is the list of "Unsafe" warts (those that are supposed to be stable) with some of them excluded:

• [ ] Any: this was causing compiler crash on the plugin sources, sometimes works strange
• [x] Any2StringAdd
• [x] AsInstanceOf
• [x] EitherProjectionPartial
• [x] IsInstanceOf
• [ ] NonUnitStatements: just not very useful, I think
• [x] Null
• [x] OptionPartial
• [x] Product
• [x] Return
• [x] Serializable
• [x] Var
• [x] ListOps

With others I just haven't had any problems so far. So this list should be reconsidered in the future. Would be especially cool to add our own wart-traversals to conform the scala-guide...

@eparejatobes what do you think?

[laughedelic]

Also I suggest the most evil and useless things to be reported as errors instead of warnings:

• Null
• EitherProjectionPartial: using get on Either
• OptionPartial: using get on Option
• Var

Not sure about

• Product
• Serializable

[eparejatobes]

@laughedelic it's not so clear to me from what you wrote which ones fail/have issues.

[laughedelic]

if you turn on Wart.Any and run wartremover on the nice-sbt-settings plugin sources, you will get a compiler crash with some funny macros messages.

[eparejatobes]

Errors

• Null
• Return
• EitherProjectionPartial: using get on Either
• OptionPartial: using get on Option
• Var
• Product
• Serializable
• ListOps
• Any2StringAdd
• AsInstanceOf
• DefaultArguments

Warnings

All else

[eparejatobes]

and the unit statements should be an error too, does it cause any problems?

[laughedelic]

I'm against of setting these as errors:

• DefaultArguments is useful enough as a warning. sometimes it's convenient to use default arguments, but it should be conscious.
• AsInstanceOf — we use it in scarph (for titan stuff) and so far we didn't find a workaround. warning is enough.
• NonUnitStatements means that you have to write { println("foo"); Unit } instead of just println("foo"), it makes a lot of inconvenience and doesn't give you much. we're not 5-years old, we know that println returns Unit.

• forbidding Return (and maybe ListOps) is too strict. you will end up with code like

  foo match {
  ... 
  bar match {
    ...
    if(qux) {
      ...
    } else {
      ...
      buh match {
                ...
                ...
              }
            }
          }
        }
      }
    }
  } 
  }

[eparejatobes]

• default arguments become convenient when your data structures lack; and they are dangerous (conversion to functions etc)
• asInstanceOf it is needed when interfacing with generic Java code; then make it a warning only in those cases
• about NonUnitStatements: what about def uhoh { 3 } which returns Unit?? I could see your point in tests (not really but hey) but certainly not in code
• Return: again, if you need that code it just means that your data types are crap

[laughedelic]

• default arguments are convenient when you don't want to write biolerplate code, but if they really ARE dangerous :gun: :bomb: :hocho:, then ok, let's forbid them.

• make it a warning only in those cases

  what does it mean? how you can check it?

• about NonUnitStatements

  I don't normally write things like def uhoh { 3 } (at least consciously).

  But in the code of this plugin, there are a lot of lines like vcs.cmd("push", vcs.trackingRemote) ! st.log (which executes an external command and logs output) inside of functions of normal type State => State, and wartremover complains about every such line.

• it just means that your data types are crap

  I don't get your point here

[eparejatobes]

The one that crashes is NoNeedForMonad, which we don't care about anyway

[eparejatobes]

about ! is a really good example actually; you're dropping the exit code there into the abyss, without any good reason for doing so.

[eparejatobes]

about the last point, give an example where you'd end up with that kind of code and I will explain what I mean

[laughedelic]

about ! is a really good example actually; you're dropping the exit code there into the abyss, without any good reason for doing so.

because I don't have a convenient way to combine such calls into a chain and I don't want to write that ... } } } } } } } } code.

My point is that your suggestions are too strict. Because if we make it all errors, every our project will fail to compile. I think that warnings for some of them are enough.

[eparejatobes]

My take on this is: If you don't have a convenient way, either build it (pretty easy in this case) or live with the inconvenience

if all of our projects fail that's good, otherwise this WartRemover thing would be useless :) Warnings are OK in those cases where you could be forced to do that, not just because of convenience.

[eparejatobes]

I think we should move all this to ohnosequences/scala-guide

[laughedelic]

I agree

[laughedelic]

I think this is done in v0.5.+

[laughedelic]

Ok, it was with warnings, now I'm making them errors