scalacenter / scalafix

Refactoring and linting tool for Scala
https://scalacenter.github.io/scalafix/
BSD 3-Clause "New" or "Revised" License
834 stars 186 forks source link

Add concept of `doc-generation-directives` #1793

Open mdedetrich opened 1 year ago

mdedetrich commented 1 year ago

In Pekko we used paradox in order to generate documentation, and the primary way that Paradox works is that you place code that you want to be documented inside of directives, i.e.

// # my-doc-code
class MyDocCode
// # my-doc-code

This means that the you can use can refer to my-doc-code in markdown documentation using @@snip Paradox directive and it would include class MyDocCode. While the above code may be trivial, in more complex scenarios of writing documentation its fairly common to "doctor" the imports and/or code so that it looks nice/readable/easy to follow when reading the generated documentation but looks terrible in the source file. https://github.com/apache/incubator-pekko-http/blob/ccd59fd804afc84926e772b150302213b6e2bf4d/docs/src/test/scala/docs/http/scaladsl/Http2Spec.scala is a good example with multiple documentation pages being generated from that single source file, i.e. https://pekko.apache.org/docs/pekko-http/current/client-side/http2.html and https://pekko.apache.org/docs/pekko-http/current/server-side/http2.html .

Initially to me, the easiest way to solve this problem would be to just provide the ability to specify what directives your code uses and also what to do in such cases. I would imagine for Paradox and/or the way that Pekko uses Paradox, in most cases the easiest would be to just ignore any code in these directives, i.e.

doc-generation-directives {
  Paradox = ignore
}

One case where ignoring code within the paradox directives is of particular important is the RemoveUnused.imports rule, i.e. https://github.com/apache/incubator-pekko-http/blob/fb224dfba74d03d88866d5c40d63ebd9729cdded/docs/src/test/scala/docs/http/scaladsl/HttpServerExampleSpec.scala you will see many cases of import org.apache.pekko but the reason why import org.apache.pekko is duplicated is because that code is being used in generated documentation and without it if someone copies code from those docs then it won't compile (because its missing import org.apache.pekko).

Although you could make an argument that certain rules can work inside the context of directives where as others not. i.e.

doc-generation-directives {
  Paradox.ignore = [
    OrganizeImports
    RemoveUnused.imports
  ]
}
SortImports.blocks = [
  ...
]

This would sort imports within the directives separately from the rest of the code base but not every organize them (since organizing the imports wouldn't make sense). However this may also have issues when nesting the Paradox directives (as mentioned before) and at least initially just having scalafix (and all of the scalafix integrations such as https://github.com/liancheng/scalafix-organize-imports) ignoring any code within a Paradox directive code block is perfectly fine.

bjaglin commented 1 year ago

Thanks for the detailed use-case! As you can imagine, I'd like to explore low-tech solutions before implementing a new feature.

Have you considered using the scalafix directives?

// scalafix:off RemoveUnused
// # my-doc-code
class MyDocCode
// # my-doc-code
// scalafix:on RemoveUnused

(if annotating this manually is too cumbersome, a local Scalafix rule could take care of wrapping the paradox directives inside scalafix ones :wink:)

Another option to handle the RemoveUnused case specifically would be to use Wconf to silence warnings emitted by the compiler, in order to effectively allow usage of repeated imports: -Wconf:cat=unused:info (or maybe to be more granular, on a specific set of sites and/or packages).

mdedetrich commented 1 year ago

Thanks for the detailed use-case! As you can imagine, I'd like to explore low-tech solutions before implementing a new feature.

Thanks for the suggestions. They definitely would work, I am not sure if polluting the entire Pekko codebase with scalafix: off would be passable (its also brittle if people modify the doc source code later) although the community would have to decide on that.

If I was to implement this as a feature, how would you design it so it makes sense for scalafix? Doing this is not urgent on our end and I feel that doing it properly would be better in the long run.

bjaglin commented 1 year ago

its also brittle if people modify the doc source code later

If important "silencing" scalafix directives are removed, the build (which I assume would run scalafixAll --check) would start failing.

If I was to implement this as a feature, how would you design it so it makes sense for scalafix?

I would replicate the logic used by paradox to identify blocks, and update https://github.com/scalacenter/scalafix/blob/3da6acdd9af4e77e40a6d9e9b88975de8b7e26a8/scalafix-core/src/main/scala/scalafix/internal/patch/EscapeHatch.scala#L206 accordingly. This would be an opt-in feature, which could be turned on either through a global boolean, or on a per-rule basis as you suggested (but that looks like a lot of complexity for a speculative ROI).

Did you consider my other suggestion based on Wconf on specs? This would unblock you until we have a more sustainable solution.