Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Reopen] Read args from file, merge args and pass to the cmd. #308

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package zio.cli

import java.io.IOException
import zio._

object ConfigFileArgsPlatformSpecific extends ConfigFilePlatformSpecific {
def findPathsOfCliConfigFiles(topLevelCommand: String): Task[List[String]] =
ZIO.succeed(Nil) // Always return empty for JS

def loadOptionsFromConfigFiles(topLevelCommand: String): ZIO[Any, IOException, List[String]] =
ZIO.succeed(Nil) // Always return empty for JS
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package zio.cli

import java.io.IOException
import zio._
import java.nio.file.{Files, Path, Paths}
import scala.io.Source

object ConfigFileArgsPlatformSpecific extends ConfigFilePlatformSpecific {
Copy link
Contributor

@Kalin-Rudnicki Kalin-Rudnicki Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be copy-pasted, if so, move into ./zio-cli/jvm-native

def findPathsOfCliConfigFiles(topLevelCommand: String): Task[List[String]] = {
val filename = s".$topLevelCommand"
val cwd = java.lang.System.getProperty("user.dir")
val homeDirOpt = java.lang.System.getProperty("user.home")
Comment on lines +11 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: zio.System


def parentPaths(path: String): List[String] = {
val parts = path.split(java.io.File.separatorChar).filterNot(_.isEmpty)
(0 to parts.length)
.map(i => s"${java.io.File.separatorChar}${parts.take(i).mkString(java.io.File.separator)}")
.toList
}
Comment on lines +14 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work for Windows/Linux/Mac?
I have a sneaking suspicion it might not work correctly for /home/kalin/dev/a/b/c/.cmd
Might be safer to implement via Path#getParent?


val paths = parentPaths(cwd)
val pathsToCheck = homeDirOpt :: homeDirOpt :: paths

// Use ZIO to filter the paths
for {
doPathExist <- ZIO.foreach(pathsToCheck)(path => ZIO.succeed(Files.exists(Path.of(path, filename))))
existingPaths = doPathExist.zip(pathsToCheck).collect { case (exists, path) if exists => path }
} yield existingPaths.distinct // Use distinct to remove duplicates at the end
}

def loadOptionsFromConfigFiles(topLevelCommand: String): ZIO[Any, IOException, List[String]] =
for {
filePaths <- findPathsOfCliConfigFiles(topLevelCommand)
.refineToOrDie[IOException]
lines <- ZIO
.foreach(filePaths) { filePath =>
ZIO.acquireReleaseWith(
ZIO.attempt(
Source.fromFile(Paths.get(filePath, "." + topLevelCommand).toFile)
)
)(source => ZIO.attempt(source.close()).orDie) { source =>
ZIO.attempt(source.getLines().toList.filter(_.trim.nonEmpty))
}
}
.map(_.flatten)
Comment on lines +35 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zio.ZIO.readFile

.refineToOrDie[IOException]
} yield lines
}
197 changes: 197 additions & 0 deletions zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
import zio._
import zio.test._
import zio.test.Assertion._
import java.nio.file.{Files, Paths, Path}
import java.io.IOException
import zio.cli.ConfigFileArgsPlatformSpecific
import zio.cli.ConfigFilePlatformSpecific
import zio.cli.Options
import zio.cli.Args
import zio.cli.Exists
import zio.cli.Command
import zio.Console.printLine
import zio.stream.ZSink
import zio.stream.ZPipeline
import zio.stream.ZStream
import zio.cli.CliApp
import zio.cli.HelpDoc.Span.text

object FileBasedArgs extends ZIOSpecDefault {

val configFileOps: ConfigFilePlatformSpecific = ConfigFileArgsPlatformSpecific

def spec = suite("FileBasedArgs")(
test("Full CLI App output test") {
for {
// Create Sample config files
cwd <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.dir")))
homeDir <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.home")))
command <- ZIO.succeed("wc")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command = "wc"


_ <- createLineCountTestFile(cwd, "sample_file")
_ <- createSampleConfigFiles(cwd, homeDir, command, content_home = "-w", content_cwd = "-l")

_ <- cliApp.run(args = List("sample_file")).either
output <- TestConsole.output

correctOutput <- ZIO.succeed(" 3 4 20 sample_file\n")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=


_ <- cleanupLineCountTestFile(cwd, "sample_file")
_ <- cleanUpSampleConfigFiles(cwd: Path, homeDir: Path, command)
} yield assert(output.last)(equalTo(correctOutput))
},
test("should load options from files and merge them appropriatly") {
for {
// Create Sample config files
cwd <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.dir")))
homeDir <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.home")))
command <- ZIO.succeed("testApp")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command = "testApp"

_ <- createSampleConfigFiles(cwd, homeDir, command)

// Check if the func checkAndGetOptionsFilePaths can
configArgs <- configFileOps.loadOptionsFromConfigFiles(command)

_ <- cleanUpSampleConfigFiles(cwd: Path, homeDir: Path, command)

} yield assert(configArgs)(hasSameElements(List("home=true", "dir=true", "home=false")))
},
test("should return directory ~/home and ./ which have .testApp config file for loading the args") {
for {
// Create Sample config files
cwd <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.dir")))
homeDir <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.home")))
command <- ZIO.succeed("testApp1")
_ <- createSampleConfigFiles(cwd, homeDir, command)

// Check if the func checkAndGetOptionsFilePaths can
paths <- configFileOps.findPathsOfCliConfigFiles(command)

_ <- cleanUpSampleConfigFiles(cwd: Path, homeDir: Path, command)

} yield assert(paths)(hasSameElements(List(homeDir.toString(), cwd.toString())))
}
)

def createLineCountTestFile(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should return

ZIO[Scope, IOException, Unit]

and clean itself up

cwd: Path,
file_name: String = "sample_file",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fileName

content: String = "asdf\nqweqwer\njdsafn"
): IO[IOException, Unit] =
ZIO.attempt {
Files.write(Paths.get(cwd.toString(), s"$file_name"), java.util.Arrays.asList(content));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

content.getBytes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s"$file_name" = fileName

()
}.refineToOrDie[IOException]

def cleanupLineCountTestFile(cwd: Path, file_name: String = "sample_file"): IO[IOException, Unit] =
ZIO.attempt {
Files.delete(Paths.get(cwd.toString(), s"$file_name"));
()
}.refineToOrDie[IOException]

def createSampleConfigFiles(
cwd: Path,
homeDir: Path,
command: String = "testApp",
content_home: String = "home=true",
content_cwd: String = "dir=true\nhome=false"
Comment on lines +95 to +96
Copy link
Contributor

@Kalin-Rudnicki Kalin-Rudnicki Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camel

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is it possible to remove the default arguments?
I find reading through the tests very hard to follow, and easily determine what output I would expect.

Also, I imagine that there should be multiple tests asserting expected behavior for different permutations of hierarchy overrides.

): IO[IOException, Unit] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing as above, writing to file should return zio Scope, and clean itself up, doing so for a single file, and be called twice

ZIO.attempt {
Files.write(Paths.get(homeDir.toString(), s".$command"), java.util.Arrays.asList(content_home));
Files.write(Paths.get(cwd.toString(), s".$command"), java.util.Arrays.asList(content_cwd));
()
}.refineToOrDie[IOException]

def cleanUpSampleConfigFiles(cwd: Path, homeDir: Path, command: String = "testApp"): IO[IOException, Unit] =
ZIO.attempt {
Files.delete(Paths.get(homeDir.toString(), s".$command"));
Files.delete(Paths.get(cwd.toString(), s".$command"));
()
}.refineToOrDie[IOException]

val bytesFlag: Options[Boolean] = Options.boolean("c")
val linesFlag: Options[Boolean] = Options.boolean("l")
val wordsFlag: Options[Boolean] = Options.boolean("w")
val charFlag: Options[Boolean] = Options.boolean("m", false)

case class WcOptions(bytes: Boolean, lines: Boolean, words: Boolean, char: Boolean)
case class WcResult(
fileName: String,
countBytes: Option[Long],
countLines: Option[Long],
countWords: Option[Long],
countChar: Option[Long]
)

val options = (bytesFlag ++ linesFlag ++ wordsFlag ++ charFlag).as(WcOptions.apply _)

val args = Args.file("files", Exists.Yes).repeat1

val wc: Command[(WcOptions, ::[Path])] = Command("wc", options, args)

val execute: (WcOptions, ::[Path]) => UIO[Unit] = {
def printResult(res: List[WcResult]): UIO[Unit] = {
def wcTotal(results: List[WcResult]) = {
def optSum(acc: WcResult, elem: WcResult, extract: WcResult => Option[Long]): Option[Long] =
extract(acc).flatMap(a => extract(elem).map(_ + a))

results.fold(WcResult("total", Some(0), Some(0), Some(0), Some(0))) { (acc, wcRes) =>
acc.copy(
countBytes = optSum(acc, wcRes, _.countBytes),
countChar = optSum(acc, wcRes, _.countChar),
countWords = optSum(acc, wcRes, _.countWords),
countLines = optSum(acc, wcRes, _.countLines)
)
}
}

def format(res: WcResult) = {
def opt(option: Option[Long]): String = option.map(l => f"$l%9d").getOrElse("")

s"${opt(res.countLines)} ${opt(res.countWords)} ${opt(res.countChar)} ${opt(res.countBytes)} ${res.fileName}"
}

ZIO.foreachDiscard(res)(r => printLine(format(r)).!) *> ZIO
.when(res.length > 1)(printLine(format(wcTotal(res))).!)
.ignore
}

(opts, paths) => {
zio.Console.printLine(s"executing wc with args: $opts $paths").! *>
ZIO
.foreachPar[Any, Throwable, Path, WcResult, List](paths)({ path =>
def option(bool: Boolean, sink: ZSink[Any, Throwable, Byte, Byte, Long])
: ZSink[Any, Throwable, Byte, Byte, Option[Long]] =
if (bool) sink.map(Some(_)) else ZSink.succeed(None)

val byteCount = option(opts.bytes, ZSink.count)
val lineCount = option(opts.lines, ZPipeline.utfDecode >>> ZPipeline.splitLines >>> ZSink.count)
val wordCount =
option(
opts.words,
ZPipeline.utfDecode >>> ZPipeline.mapChunks((_: Chunk[String]).flatMap(_.split("\\s+"))) >>> ZSink.count
)
val charCount =
option(opts.char, ZPipeline.utfDecode >>> ZSink.foldLeft[String, Long](0L)((s, e) => s + e.length))

val zippedSinks
: ZSink[Any, Throwable, Byte, Byte, (Option[Long], Option[Long], Option[Long], Option[Long])] =
(byteCount <&> lineCount <&> wordCount <&> charCount)

ZStream
.fromFile(path.toFile)
.run(zippedSinks)
.map(t => WcResult(path.getFileName.toString, t._1, t._2, t._3, t._4))
})
.withParallelism(4)
.orDie
.flatMap(res => printResult(res))
}
}

val cliApp = CliApp.make[Any, Throwable, (WcOptions, ::[Path]), Unit](
"ZIO Word Count",
"0.1.2",
text("counts words in the file"),
wc
)(execute.tupled)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package zio.cli

import java.io.IOException
import zio._
import java.nio.file.{Files, Path, Paths}
import scala.io.Source

object ConfigFileArgsPlatformSpecific extends ConfigFilePlatformSpecific {
def findPathsOfCliConfigFiles(topLevelCommand: String): Task[List[String]] = {
val filename = s".$topLevelCommand"
val cwd = java.lang.System.getProperty("user.dir")
val homeDirOpt = java.lang.System.getProperty("user.home")

def parentPaths(path: String): List[String] = {
val parts = path.split(java.io.File.separatorChar).filterNot(_.isEmpty)
(0 to parts.length)
.map(i => s"${java.io.File.separatorChar}${parts.take(i).mkString(java.io.File.separator)}")
.toList
}

val paths = parentPaths(cwd)
val pathsToCheck = homeDirOpt :: homeDirOpt :: paths

// Use ZIO to filter the paths
for {
doPathExist <- ZIO.foreach(pathsToCheck)(path => ZIO.succeed(Files.exists(Path.of(path, filename))))
existingPaths = doPathExist.zip(pathsToCheck).collect { case (exists, path) if exists => path }
} yield existingPaths.distinct // Use distinct to remove duplicates at the end
}

def loadOptionsFromConfigFiles(topLevelCommand: String): ZIO[Any, IOException, List[String]] =
for {
filePaths <- findPathsOfCliConfigFiles(topLevelCommand)
.refineToOrDie[IOException]
lines <- ZIO
.foreach(filePaths) { filePath =>
ZIO.acquireReleaseWith(
ZIO.attempt(
Source.fromFile(Paths.get(filePath, "." + topLevelCommand).toFile)
)
)(source => ZIO.attempt(source.close()).orDie) { source =>
ZIO.attempt(source.getLines().toList.filter(_.trim.nonEmpty))
}
}
.map(_.flatten)
.refineToOrDie[IOException]
} yield lines
}
35 changes: 23 additions & 12 deletions zio-cli/shared/src/main/scala/zio/cli/CliApp.scala
Original file line number Diff line number Diff line change
Expand Up @@ -128,19 +128,30 @@ object CliApp {
case Command.Subcommands(parent, _) => prefix(parent)
}

self.command
.parse(prefix(self.command) ++ args, self.config)
.foldZIO(
e => printDocs(e.error) *> ZIO.fail(CliError.Parsing(e)),
{
case CommandDirective.UserDefined(_, value) =>
self.execute(value).map(Some(_)).mapError(CliError.Execution(_))
case CommandDirective.BuiltIn(x) =>
executeBuiltIn(x).catchSome { case err @ CliError.Parsing(e) =>
printDocs(e.error) *> ZIO.fail(err)
}
// Reading args from config files and combining with provided args
val combinedArgs: ZIO[R, CliError[E], List[String]] =
ConfigFileArgsPlatformSpecific
.loadOptionsFromConfigFiles(self.command.names.head)
.flatMap { configArgs =>
ZIO.succeed(configArgs ++ args)
}
Comment on lines +135 to 137
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.map

)
.mapError(e => CliError.IO(e))

combinedArgs.flatMap { allArgs =>
self.command
.parse(prefix(self.command) ++ allArgs, self.config)
.foldZIO(
e => printDocs(e.error) *> ZIO.fail(CliError.Parsing(e)),
{
case CommandDirective.UserDefined(_, value) =>
self.execute(value).map(Some(_)).mapError(CliError.Execution(_))
case CommandDirective.BuiltIn(x) =>
executeBuiltIn(x).catchSome { case err @ CliError.Parsing(e) =>
printDocs(e.error) *> ZIO.fail(err)
}
}
)
}
}

override def flatMap[R1 <: R, E1 >: E, B](f: A => ZIO[R1, E1, B]): CliApp[R1, E1, B] =
Expand Down
9 changes: 9 additions & 0 deletions zio-cli/shared/src/main/scala/zio/cli/ConfigFileArgs.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package zio.cli

import zio._
import java.io.IOException

trait ConfigFilePlatformSpecific {
def findPathsOfCliConfigFiles(topLevelCommand: String): Task[List[String]]
def loadOptionsFromConfigFiles(topLevelCommand: String): ZIO[Any, IOException, List[String]]
}
Comment on lines +6 to +9
Copy link
Contributor

@Kalin-Rudnicki Kalin-Rudnicki Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal opinion:
I think it would be nicer if this passed via something ~like

trait ConfigFileArgs {
  // ...
}
object ConfigFileArgs {
  val default: ConfigFileArgs // platform specific
}
sealed trait CliApp[-R, +E, +A] { self =>
  def runWithFileArgs(args: List[String]): ZIO[R & ConfigFileArgs, CliError[E], Option[A]]
  final def run(args: List[String]): ZIO[R, CliError[E], Option[A]] = 
    runWithFileArgs(args).provideSomeLayer(ZLayer.succeed(ConfigFileArgs.default))
}

Copy link
Contributor

@Kalin-Rudnicki Kalin-Rudnicki Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I suggest this is because it would allow for more separation of concerns, and make much more testable.
You could independently test a Live impl, verify that is doing what it should, and provide a Mock layer to the actual cliApp.run (or in this example cliApp.runWithFileArgs), so that you arent doing all this file standup/teardown in order to test the internals of the parsing is working correctly

Again, very much personal opinion 😅

Loading