Add life-cycle methods to PostProcessor modifiers #245
Conversation
…suites. Added some documentation.
…he new life-cycle methods of the PostModifier trait.
…nternal Settings class.
…nternal Settings class.
| @@ -171,7 +171,9 @@ class Processor(implicit ctx: Context) { | |||
| inputFile, | |||
| ctx.settings | |||
| ) | |||
| modifier.preProcess(postCtx) | |||
| val postRender = modifier.process(postCtx) | |||
There was a problem hiding this comment.
What's the purpose of preProcess and postProcess if the modifier could call those methods directly inside of process? Judging by the implementation, it doesn't look like mdoc does anything between preProcess, process and postProcess
class MyProcess extends PostProcess {
def preProcess() = ()
def process() = {
preProcess()
// ...
postProcess()
}
}There was a problem hiding this comment.
You are right. This is incorrect. Does nothing. I think these calls should be removed. Ok with you? Want to change the onStart and onExit names?
|
Thank you for this contribution! Can you please update the description to explain the motivation for this change, something like
|
olafurpg
changed the title
…om PostModifier. PostModifier.onExit now receives an Int as the exit code.
mdoc/src/main/scala/mdoc/Exit.scala
Outdated
| } | ||
|
|
||
| object Exit { | ||
| def apply(exit: CliExit): Exit = { |
There was a problem hiding this comment.
I think we can remove this method, or make it package private
There was a problem hiding this comment.
Had to make this package private because it is used here in MainOps.
| // Global thread local counter updated by all mdoc.Main process | ||
| // All tests in this test suite run sequentially but change the counter | ||
| // So make sure we start anew for this test | ||
| LifeCycleCounter.numberOfStarts.set(0) |
There was a problem hiding this comment.
Can we introduce some reset() helper method to do this?
| @@ -46,4 +49,132 @@ abstract class BaseCliSuite extends FunSuite with DiffAssertions { | |||
| onStdout(stdout) | |||
| } | |||
| } | |||
|
|
|||
| def assertOneSameOrPrintExpected(obtained: String, expected: List[String], title: String = "")( | |||
There was a problem hiding this comment.
Why do we need to copy-paste so much code?
There was a problem hiding this comment.
As per the comments (see lines 72 and 73) the scala.meta.testkit.DiffAssertions methods that I needed cannot be accessed because they are private. These belong to the testkit package. If these were accessible much of the copy & paste is not required. Alternatively the assertOneSameOrPrintExpectedcould be placed in the testkitpackage.
Seeing as it is not part of MDoc I opted for this solution. Any way testkit can be changed?
There was a problem hiding this comment.
What functionality is missing? We should avoid copy pasting this amount of code
There was a problem hiding this comment.
Not required anymore so it was removed. See my explanation below. Initially I used this to test the pre and post processing methods. You correctly pointed out they were not necessary. So I removed them but did not correct the test.
…CycleCounter now has a reset method that is used in the CliSuite test.
| } | ||
| } | ||
|
|
||
| def checkCliMulti( |
There was a problem hiding this comment.
Did you try using the checkCli method? It supports processing multiple files. I'm curious to understand why this method is needed
There was a problem hiding this comment.
The problem is the ordering in which the files are processed. Assume files A and B. If A gets processed before B, then A gets a count of 1 and B a 2. If the order is reversed then the B gets count 1 and A gets 2. The order in which the files are processed in my local machine and in the CI/CD VM were different. So I crated a checkCliMulti method, which checks that at least one of the conditions I stipulate is true. I suspect that the transformation tests you have don't have this issue. Maybe I don't need this?
There was a problem hiding this comment.
I have just realized I don't need this test. I was using it when I had the pre and post process that were removed. So I am removing this.
|
I haven't forgotten about this PR. There are a few organizational changes that I'd like to make to the codebase before merging this PR so it may take a while longer. I have a feeling we may need to bump to v3.0 due to unavoidable binary incompatibilities but I would still like to keep the breaking changes as small as possible. The reason I care about keeping a stable API so much is that I want people to be able to publish their mdoc modifiers without having to re-publish on every small change in the mdoc API. |
Before, mdoc would instantiate and execute
PostModifierinstances without any indication when these terminated their processing on all documents. This means that if we created some resources in these PostModfiers, such as instantiating a daemon thread, there was no way of releasing these resources. In the case of daemon threads, this meant that a PostModifer could prevent the CLI call from terminating.Now, mdoc call the
onStartmethods jus before the files are processed andonExitjust before mdoc terminates. This gives the PostModifier instances an opportunity to create and release any required resources. For example the daemon thread can be signaled to terminateonExitthereby allowing the mdoc CLI to terminate.Changes for #213