Make Evaluator and EvaluatorState normal classes #1740
Conversation
lolgab
force-pushed
the
evaluator-api-change
branch
from
68006e9
to
bd42145
Compare
| failFast: Boolean = true, | ||
| threadCount: Option[Int] = Some(1) | ||
| ) { | ||
| class Evaluator @deprecated(message = "Use apply instead", since = "0.10.1") ( |
There was a problem hiding this comment.
When a new field gets added to the class, this constructor needs to be moved to a separate def this and the main constructor needs to be made private
There was a problem hiding this comment.
Looks reasonable to me.
I'd prefer to include the word "mill" in the @deprecated since attribute, to improve reading of compiler deprecation messages. Note, that the compiler just spits out all deprecation warnings as they occur and as a user it can be hard to guess where these are coming from and what dependency could be the culprit (esp. when transitive). So I'd go with the convention to always include the library name. The "after x.y.z" might be a bit weird, but it's actually the most accurate message we can leave without the need for later maintenance edits.
Example:
@deprecated("...", "mill 0.10.1")
// or if the next version is still unclear
@deprecated("...", "mill after 0.10.0")
/** @since Mill after 0.10.0 */
|
If we merge this PR now (befor the release of |
Sure. Just add the "mill" then. I wanted to explain my whole reasoning, as I had situations before, e.g. in the late 0.9 and early 0.10 phase. |
lolgab
force-pushed
the
evaluator-api-change
branch
from
cb64dad
to
65798d7
Compare
Extracted from #1663 to allow introducing new fields in the Evaluator and EvaluatorState classes without breaking the binary compatibility.