Target a single file for `--in` and `--out` #322

ckipp01 · 2020-04-20T12:49:38Z

This pr introduces the ability to target an individual file with --in and also --out. If an individual file is passed to --out it also does a check to ensure that --in is also an individual file.

This will allow the use to specify a target without having to worry about globs or anything. It becomes much easier to do things like this like below where extras may have various files, but you only want one.

❯ cs launch org.scalameta:mdoc_2.12:2.1.5+13-c957fe2e-SNAPSHOT -- --in extras/readme.md

or

❯ cs launch org.scalameta:mdoc_2.12:2.1.5+13-c957fe2e-SNAPSHOT -- --in extras/readme.template.md --out readme.md

For the future

~~Allow --out to also be an individual file (it would require --in to also be a file), which would allow you to for example target --in /extras/readme.template.md --out readme.md.~~
Maybe a "File Name Modifer" of sorts that would allow you to extend it and provide a method to change the name of files that are passed through mdoc in a specified way.

olafurpg

Thank you for this contribution! Can we add a test?

docs/installation.md

mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala

mdoc/src/main/scala/mdoc/internal/cli/MainOps.scala

mdoc/src/main/scala/mdoc/internal/cli/Settings.scala

ckipp01 · 2020-04-20T15:17:55Z

Thank you for this contribution! Can we add a test?

I'm having a hard time figuring out what the best way to do this is. At first I created a SingleFileCliSuite that extended the BaseCli suite in hopes that I could take all of the tests in CliSuite and do a super.checkCli(...) which would check them all as being passed in as a directory, and then the SingleFileCliSuite checkCli would check them as an individual file. However, the way that we pass in original to StringFS.creatTempDirectory is really tightly coupled to that original format and at times those tests have multiple files represented in the string.

Then I thought about doing an entirely new Suite with a check where I still create the tempDirectory and explicitly pass it the file name I wanted to do the check, but then there would be a lot of repetition from stuff that is done in CliSuite.scala, but only slightly tweaked to only have 1 file represented and passed to the check. I more than likely would do something like this:

  checkSingleFileCli(
    "blah", <- name 
	"index.md", <- file to check to add to what StringFS.fromString(...) returns
    """
      |/index.md
      |```scala mdoc
      |List(1,
      |  2, 3, 4) // comment
      |```
    """.stripMargin,
    """
      |/index.md
      |```scala
      |List(1,
      |  2, 3, 4) // comment
      |// res0: List[Int] = List(1, 2, 3, 4)
      |```
    """.stripMargin
  )

Is that the best way to do this? Or is there a better way to re-utilize what we have? How would you recommend testing this?

olafurpg · 2020-04-20T18:32:14Z

I think we can reuse the same method by adding a new parameter to configure the --in flag. Something like this

diff --git a/tests/unit/src/test/scala/tests/cli/BaseCliSuite.scala b/tests/unit/src/test/scala/tests/cli/BaseCliSuite.scala
index fa71e35..8edbeca 100644
--- a/tests/unit/src/test/scala/tests/cli/BaseCliSuite.scala
+++ b/tests/unit/src/test/scala/tests/cli/BaseCliSuite.scala
@@ -19,6 +19,7 @@ abstract class BaseCliSuite extends FunSuite {
       expected: String,
       extraArgs: Array[String] = Array.empty,
       setup: CliFixture => Unit = _ => (),
+      configureInFlag: AbsolutePath => String = _.toString(),
       expectedExitCode: Int = 0,
       onStdout: String => Unit = _ => ()
   ): Unit = {
@@ -29,7 +30,7 @@ abstract class BaseCliSuite extends FunSuite {
       setup(CliFixture(in.toNIO, out))
       val args = Array[String](
         "--in",
-        in.toString,
+        configureInFlag(in),
         "--out",
         out.toString,
         "--cwd",

Also bump the docusaurus version

olafurpg

Allow --out to also be an individual file (it would require --in to also be a file), which would allow you to for example target --in /extras/readme.template.md --out readme.md

How hard would it be to implement this? I think it would be nice to require --out to be a valid filename (!Files.isDirectory(out)) when --in is a filename. We can perform that validation here

mdoc/mdoc/src/main/scala/mdoc/internal/cli/Settings.scala

Line 185 in 30dec31

if (Files.exists(in.toNIO)) {

tests/unit/src/test/scala/tests/cli/CliSuite.scala

ckipp01 · 2020-04-21T10:56:37Z

Allow --out to also be an individual file (it would require --in to also be a file), which would allow you to for example target --in /extras/readme.template.md --out readme.md

How hard would it be to implement this? I think it would be nice to require --out to be a valid filename (!Files.isDirectory(out)) when --in is a filename. We can perform that validation here

Do you think it should be required though? I can see if you pass in an individual file to --out that you'd want to ensure that the --in is also a file, but shouldn't the other way around be able to do either? More often then not I imagine you'd provide both, but I don't see a good reason to restrict it to that? What do you think?

Also, I'll start working on the --out being a file and include it in this pr.

olafurpg · 2020-04-21T11:17:25Z

Fair enough, I agree it's not necessary to fail if --out is a directory and --in is a file.

ckipp01 · 2020-04-25T19:39:53Z

Fair enough, I agree it's not necessary to fail if --out is a directory and --in is a file.

Cool, that's the way I implemented it. I think this turned out alright! I'll give a couple comments on relevant parts.

mdoc/src/main/scala/mdoc/internal/cli/Settings.scala

ckipp01 · 2020-04-25T19:44:58Z

mdoc/src/main/scala/mdoc/internal/cli/Settings.scala

-  def resolveIn(relpath: RelativePath): AbsolutePath = {
-    in.resolve(relpath)
-  }

-  def resolveOut(relpath: RelativePath): AbsolutePath = {
-    out.resolve(relpath)


I removed these since they were unused.

olafurpg · 2020-05-04T11:39:53Z

Following up from an offline conversation, I took the liberty to push a commit that makes several changes

support a list of input/output pairs so it's possible to process multiple individual files (example: readme.md and changelog.md)
updates the custom modifier APIs to properly expose individual files
extend Scala.js modifier to handle individual files, it needs an output directory + relative path to work correctly with Docusaurus
extend the file watcher to handle individual files while avoiding performance issues when starting mdoc in file watching mode
various small cleanups
- fix parsing of relative filenames when using a custom working directory via --cwd, this makes it easier to run ad-hoc manual tests from the mdoc sbt builld
- use MUnit Fixtures to make it easier to reference the input/output arguments in assertions
- make it possible to test the Scala.js modifier in BaseCliSuite, previously it only used BaseMarkdownSuite which is a synthetic environment that's not the same as when using sbt-mdoc.

Previously, mdoc only supported a single --in and --out argument and they both had to be directories. Now, users can specify a list of --in and --out arguments of both files and directories. This change enables nice use-cases like generating a single `readme.md` file in the root directory of a git repo or generating the blog directory of a Docusaurus website. ``` mdoc --in readme.template.md --out readme.md mdoc --in docs --out website/target/docs --in blog --out website/blog ``` This change turned out to be tricky to implement: * the public API for custom modifiers needs to correctly handle the case when --in and --out are files. For example, the Scala.js modifier needs to be able to know where to generate JavaScript files. * we need to report helpful and actionable error messages when validating user input. For example, it's not valid to pair an input directory with an output file.

olafurpg

LGTM 👍 I think this is ready for merge once CI is green..

@ckipp01 can you take a look to see if everything is still working as expected?

ckipp01 · 2020-05-04T13:38:17Z

build.sbt

@@ -36,7 +37,8 @@ inThisBuild(
    // faster publishLocal:
    publishArtifact.in(packageDoc) := "true" == System.getenv("CI"),
    publishArtifact.in(packageSrc) := "true" == System.getenv("CI"),
-    turbo := true
+    turbo := true,


TIL'd about turbo 🤔

ckipp01 · 2020-05-04T13:39:26Z

docs/installation.md

+
+### Process multiple input directories and files
+
+Repeat the `--in` and `--out` arguments to process multiple directories and


mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala

mdoc/src/main/scala/mdoc/internal/cli/Feedback.scala

mdoc/src/main/scala/mdoc/internal/cli/MainOps.scala

mdoc/src/main/scala/mdoc/internal/cli/Settings.scala

mdoc/src/main/scala/mdoc/internal/io/IO.scala

ckipp01 · 2020-05-04T13:59:23Z

mdoc/src/main/scala/mdoc/internal/io/MdocFileListener.scala

+      // on startup for large directories (can take minutes). To prevent
+      // duplicate notifications, we implement file hashing only for files the
+      // files we're interested in, see MainOps.
+      .fileHashing(false)


add in the ability for --in to handle a single file

c957fe2

olafurpg requested changes Apr 20, 2020

View reviewed changes

ckipp01 force-pushed the single-file branch from 585c350 to 0008776 Compare April 20, 2020 16:35

add in documenation for targeting an individual file

5a5111c

Also bump the docusaurus version

ckipp01 force-pushed the single-file branch from 0008776 to 5a5111c Compare April 21, 2020 09:09

ckipp01 requested a review from olafurpg April 21, 2020 09:10

olafurpg requested changes Apr 21, 2020

View reviewed changes

tests/unit/src/test/scala/tests/cli/CliSuite.scala Outdated Show resolved Hide resolved

tests/unit/src/test/scala/tests/cli/CliSuite.scala Show resolved Hide resolved

add ability to specify an out file

264ee02

ckipp01 force-pushed the single-file branch from 988bc96 to 264ee02 Compare April 25, 2020 19:36

ckipp01 requested a review from olafurpg April 25, 2020 19:39

ckipp01 commented Apr 25, 2020

View reviewed changes

mdoc/src/main/scala/mdoc/internal/cli/Settings.scala Show resolved Hide resolved

ckipp01 changed the title ~~Target a single file~~ Target a single file for --in and --out Apr 25, 2020

ckipp01 commented Apr 25, 2020

View reviewed changes

olafurpg force-pushed the single-file branch from 8343379 to 20e71df Compare May 4, 2020 11:45

Add more tests and docs

1150de9

olafurpg force-pushed the single-file branch from b7c4ba3 to 1150de9 Compare May 4, 2020 12:47

olafurpg approved these changes May 4, 2020

View reviewed changes

ckipp01 commented May 4, 2020

View reviewed changes

Address review comments

6ba97f7

olafurpg merged commit 64f66d0 into scalameta:master May 5, 2020

ckipp01 deleted the single-file branch May 5, 2020 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target a single file for `--in` and `--out` #322

Target a single file for `--in` and `--out` #322

ckipp01 commented Apr 20, 2020 •

edited

Loading

olafurpg left a comment

ckipp01 commented Apr 20, 2020 •

edited

Loading

olafurpg commented Apr 20, 2020

olafurpg left a comment

ckipp01 commented Apr 21, 2020

olafurpg commented Apr 21, 2020

ckipp01 commented Apr 25, 2020

ckipp01 Apr 25, 2020

olafurpg commented May 4, 2020 •

edited

Loading

olafurpg left a comment

ckipp01 May 4, 2020

ckipp01 May 4, 2020

ckipp01 May 4, 2020


		### Process multiple input directories and files

		Repeat the `--in` and `--out` arguments to process multiple directories and

Target a single file for --in and --out #322

Target a single file for --in and --out #322

Conversation

ckipp01 commented Apr 20, 2020 • edited Loading

For the future

olafurpg left a comment

Choose a reason for hiding this comment

ckipp01 commented Apr 20, 2020 • edited Loading

olafurpg commented Apr 20, 2020

olafurpg left a comment

Choose a reason for hiding this comment

ckipp01 commented Apr 21, 2020

olafurpg commented Apr 21, 2020

ckipp01 commented Apr 25, 2020

ckipp01 Apr 25, 2020

Choose a reason for hiding this comment

olafurpg commented May 4, 2020 • edited Loading

olafurpg left a comment

Choose a reason for hiding this comment

ckipp01 May 4, 2020

Choose a reason for hiding this comment

ckipp01 May 4, 2020

Choose a reason for hiding this comment

ckipp01 May 4, 2020

Choose a reason for hiding this comment

Target a single file for `--in` and `--out` #322

Target a single file for `--in` and `--out` #322

ckipp01 commented Apr 20, 2020 •

edited

Loading

ckipp01 commented Apr 20, 2020 •

edited

Loading

olafurpg commented May 4, 2020 •

edited

Loading