Scalapb: Generated scala code uses Option.get

Created on 22 Nov 2017  路  3Comments  路  Source: scalapb/ScalaPB

In general Option.get is a bad practice:
https://alvinalexander.com/scala/best-practice-option-some-none-pattern-scala-idioms#don-t-use-the-get-method-with-option

It's basically the same as a normal value that just throws a null pointer exception.

Practically, using Option.get causes the auto generated code to fail linters like WartRemover

It would be nice to change the code generation to, as a first step, simply call Option.getOrElse(throw new SensibleException()), or ideally use the pattern

Option match { case Some(x) => x; case None => handleFailureCase() }

Doing the first simple case at least solves the problem with linting

Most helpful comment

In general, auto-generated code doesn't have to completely satisfy all style guides or linters recommendations that apply to normal user code. For example, we're not trying to target any specific line width but we're trying to make the generator keep it reasonable.

For this specific case we have something like this: if (opt.isDefined) { __size ++ = something(opt.get) }. The most idiomatic thing to use would be to use foreach, but that would mean an extra function call and probably and a function object for each optional field. Pattern matching comes second, but I find it actually less desirable than the current implementation for the case when there's just one case. We'll have to add case None => // for avoiding a warning on the match not being exhaustive.

My preference here would be to check if tools such as wart remover can skip linting the generated code. Happy to add any comment to the top of the generated code so they won't try to lint those files.

All 3 comments

In general, auto-generated code doesn't have to completely satisfy all style guides or linters recommendations that apply to normal user code. For example, we're not trying to target any specific line width but we're trying to make the generator keep it reasonable.

For this specific case we have something like this: if (opt.isDefined) { __size ++ = something(opt.get) }. The most idiomatic thing to use would be to use foreach, but that would mean an extra function call and probably and a function object for each optional field. Pattern matching comes second, but I find it actually less desirable than the current implementation for the case when there's just one case. We'll have to add case None => // for avoiding a warning on the match not being exhaustive.

My preference here would be to check if tools such as wart remover can skip linting the generated code. Happy to add any comment to the top of the generated code so they won't try to lint those files.

I'll look in to options for my nix/pants environment at the wartremover layer

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ngbinh picture ngbinh  路  4Comments

nykolaslima picture nykolaslima  路  9Comments

jportway picture jportway  路  13Comments

ghost picture ghost  路  3Comments

dhuadaar picture dhuadaar  路  13Comments