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

vivasvan1 · 2024-04-18T08:48:59Z

/claim #191

closes #191

Hi @jdegoes,

Hi have attempted to resolve your review comments on the previous PR

Please let me know if more changes are required.

Working Screenshots:

References:

Old PR: Read args from file, merge args and pass to the cmd. #278

P.S. : I dont have permission to reopen the previous PR so created a fresh one.

CLAassistant · 2024-04-18T08:49:05Z

All committers have signed the CLA.

vivasvan1 · 2024-04-18T10:31:34Z

Also could i please get provide some guidance in fixing CI/CD error. I am new to this ecosystem and unclear on what exactly the errors are.

jdegoes · 2024-04-19T09:47:55Z

[error] Modules were resolved with conflicting cross-version suffixes in ProjectRef(uri("file:/home/runner/work/zio-cli/zio-cli/"), "zioCliJVM"):
[error]    org.scala-lang.modules:scala-collection-compat _3, _2.13
[error] java.lang.RuntimeException: Conflicting cross-version suffixes in: org.scala-lang.modules:scala-collection-compat

This error is related to Scala JS, to use of a library scala-collection-compat. Probably version numbers for the Scala compiler(s) need updating to be consistent with each other. I would look in the build file.

jdegoes

Looks good! Please just add a test to ensure the behavior is working correctly, and th en we can merge!

As of 19th April zio-nio doesnt support scalaJS so using java to read file and zio core for resource management

vivasvan1 · 2024-04-19T14:21:58Z

Update: 22 Apr 2024:
✅ All Green)) - @jdegoes

Notes:

JS support can be added later.

Update 20 Apr 2024:
Tests are also initiallized. Let me know if any changes are required.

Looks good! Please just add a test to ensure the behavior is working correctly, and th en we can merge!

@jdegoes
Update 19 Apr 2024:
Hi there,

First, thank you for your review.

I have commited fix for build errors in JVM and Native.

However, errors are now only in scalaJS.
Error are only of this type:

I believe it is JS doesnt support java.io
If you know about this could you guide me a little?

Also, I'm working on writing test. should be done soon.

Thank you again :)

vivasvan1 · 2024-05-05T12:11:24Z

@jdegoes
Hi there, hope this message finds you well.
Reminder: I have fixed the issues you mentioned.
Could you please let me know how to proceed?
Thanks :)

pablf · 2024-05-06T16:05:53Z

@vivasvan1 Hi! The tests do not cover the case of a real CliApp. Testing locally the following test suite, it seems to be failing. I don't know if there might be some other issues. Method parse of Command is the one that parse the parameters. I think that the easiest way to solve it would be to modify this method so that it accepts separately the user parameters and the file parameters and then process them knowing to which group each one belongs instead of just concatenating them.

If you could add a test suite similar to this testing the whole process of running the CliApp it would be great!

import zio._
import zio.test._
import zio.test.Assertion._
import java.nio.file.{Files, Paths, Path}
import java.io.IOException
import zio.cli._

object AlternativeSpec extends ZIOSpecDefault {

  val configFileOps: ConfigFilePlatformSpecific = ConfigFileArgsPlatformSpecific

  def spec = suite("FileBasedArgs")(
    test("failing test 1") {
      for {
        // Create Sample config files
        cwd     <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.dir")))
        _       <- createSampleConfigFiles2(cwd, "someCommand")
        cliApp = CliApp.make(
          name = "cliApp",
          version = "0",
          summary = HelpDoc.Span.empty,
          command = Command("someCommand", Options.text("opt")),
        ) {
          case text: String => ZIO.succeed(text)
        }
        res <- cliApp.run(List("--opt", "inputText"))

        // Check if the func checkAndGetOptionsFilePaths can

        _ <- cleanUpSampleConfigFiles2(cwd, "someCommand")

      } yield assertTrue(res == Some("inputText"))
    },
    test("failing test 2") {
      for {
        // Create Sample config files
        cwd     <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.dir")))
        command = "someCommandB"
        _       <- createSampleConfigFiles2(cwd, command)
        cliApp = CliApp.make(
          name = "cliApp",
          version = "0",
          summary = HelpDoc.Span.empty,
          command = Command(command, Options.text("opt"), Args.integer("num")),
        ) {
          case text => ZIO.succeed(text)
        }
        res <- cliApp.run(List("--opt", "inputText", "4"))

        // Check if the func checkAndGetOptionsFilePaths can

        _ <- cleanUpSampleConfigFiles2(cwd, command)

      } yield assertTrue(res == Some(("inputText", BigInt(4))))
    },
    test("failing test 3") {
      for {
        // Create Sample config files
        cwd     <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.dir")))
        command = "someCommandCLeft"
        command2 = "someCommandCRight"
        _       <- createSampleConfigFiles2(cwd, command)
        cliApp = CliApp.make(
          name = "cliApp",
          version = "0",
          summary = HelpDoc.Span.empty,
          command = Command(command, Options.text("opt"), Args.integer("num")) | Command(command2, Options.text("opt"), Args.integer("num")),
        ) {
          case text => ZIO.succeed(text)
        }
        res <- cliApp.run(List(command, "4"))

        
        // Check if the func checkAndGetOptionsFilePaths can

        _ <- cleanUpSampleConfigFiles2(cwd, command)

      } yield assertTrue(res == Some(("fileText", BigInt(4))))
    },
    test("failing test 4") {
      for {
        // Create Sample config files
        cwd     <- ZIO.succeed(Paths.get(java.lang.System.getProperty("user.dir")))
        command = "someCommandD"
        _       <- createSampleConfigFiles2(cwd, command)
        cliApp = CliApp.make(
          name = "cliApp",
          version = "0",
          summary = HelpDoc.Span.empty,
          command = Command(command, Args.integer("num")),
        ) {
          case text => ZIO.succeed(text)
        }
        res <- cliApp.run(List("4"))

        // Check if the func checkAndGetOptionsFilePaths can

        _ <- cleanUpSampleConfigFiles2(cwd, command)

      } yield assertTrue(res == Some(BigInt(4)))
    },
  )

  def createSampleConfigFiles2(cwd: Path, command: String = "testApp"): IO[IOException, Unit] =
    ZIO.attempt {
      Files.write(Paths.get(cwd.toString(), s".$command"), java.util.Arrays.asList("--opt=fileText"));

      ()
    }.refineToOrDie[IOException]

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

      ()
    }.refineToOrDie[IOException]
}

vivasvan1 · 2024-06-04T08:13:03Z

@pablf Hi there,

I have added the test case you asked for. Where the functionality is being tested on the full wc app.
Let me know if any futher changes are required.

I am not sure why your local is failing. Could you provide more details.
i tried to understand the parse function but as i am new to zio, scala it felt bit overwhelming.

Thanks,
Vivasvan

Kalin-Rudnicki · 2024-06-04T12:25:22Z

zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala

+    }
+  )
+
+  def createLineCountTestFile(


This should return

ZIO[Scope, IOException, Unit]

and clean itself up

Kalin-Rudnicki · 2024-06-04T12:26:10Z

zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala

+
+  def createLineCountTestFile(
+    cwd: Path,
+    file_name: String = "sample_file",


Kalin-Rudnicki · 2024-06-04T12:26:24Z

zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala

+    content_home: String = "home=true",
+    content_cwd: String = "dir=true\nhome=false"


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.

Kalin-Rudnicki · 2024-06-04T12:28:59Z

zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala

+    command: String = "testApp",
+    content_home: String = "home=true",
+    content_cwd: String = "dir=true\nhome=false"
+  ): IO[IOException, Unit] =


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

Kalin-Rudnicki · 2024-06-04T12:29:36Z

zio-cli/jvm/src/main/scala/zio/cli/ConfigFileArgsPlatformSpecific.scala

+    val cwd        = java.lang.System.getProperty("user.dir")
+    val homeDirOpt = java.lang.System.getProperty("user.home")


See: zio.System

Kalin-Rudnicki · 2024-06-04T12:30:11Z

zio-cli/jvm/src/main/scala/zio/cli/ConfigFileArgsPlatformSpecific.scala

+      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)


zio.ZIO.readFile

Kalin-Rudnicki · 2024-06-04T12:33:26Z

zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala

+        // 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")


command = "testApp"

Kalin-Rudnicki · 2024-06-04T12:33:42Z

zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala

+        // 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")


command = "wc"

Kalin-Rudnicki · 2024-06-04T12:33:59Z

zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala

+        _      <- cliApp.run(args = List("sample_file")).either
+        output <- TestConsole.output
+
+        correctOutput <- ZIO.succeed("        3         4        20  sample_file\n")


Kalin-Rudnicki · 2024-06-04T12:36:14Z

zio-cli/shared/src/main/scala/zio/cli/CliApp.scala

+          .flatMap { configArgs =>
+            ZIO.succeed(configArgs ++ args)
          }


Kalin-Rudnicki · 2024-06-04T12:37:22Z

zio-cli/jvm/src/main/scala/zio/cli/ConfigFileArgsPlatformSpecific.scala

+import java.nio.file.{Files, Path, Paths}
+import scala.io.Source
+
+object ConfigFileArgsPlatformSpecific extends ConfigFilePlatformSpecific {


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

Kalin-Rudnicki · 2024-06-04T12:40:45Z

zio-cli/jvm/src/test/scala/zio/cli/FileBasedArgs.scala

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


content.getBytes

s"$file_name" = fileName

Kalin-Rudnicki · 2024-06-04T13:17:14Z

zio-cli/jvm/src/main/scala/zio/cli/ConfigFileArgsPlatformSpecific.scala

+    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
+    }


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?

Kalin-Rudnicki · 2024-06-04T13:24:27Z

zio-cli/shared/src/main/scala/zio/cli/ConfigFileArgs.scala

+trait ConfigFilePlatformSpecific {
+  def findPathsOfCliConfigFiles(topLevelCommand: String): Task[List[String]]
+  def loadOptionsFromConfigFiles(topLevelCommand: String): ZIO[Any, IOException, List[String]]
+}


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)) }

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 😅

vivasvan1 · 2024-06-04T14:56:42Z

@Kalin-Rudnicki
Wow, thank you so much for all the comments, I really appreciate your help 🙏.

I will look into this asap. This seems really helpful.

pablf · 2024-06-04T17:52:51Z

@vivasvan1 If I remember correctly, the tests contained in the code of my last comment should all pass if the implementation of the feature were correct, so if you copy that code into zio-cli/jvm/src/test/scala/zio/cli/AlternativeSpec.scala it should be green. It's great testing now on CliApp as you have changed, but I only see one use case tested. I think this is the problem. My sample test should be working (correct me if I'm wrong please) so, would you mind adding it exactly the same to your PR? If it passed, maybe there would be some other test that should be working and isn't, but I think this would be a good start.

If you have any doubt, ask me please! Or maybe you have already tried adding the AlternativeSpec to your build and was passing? Given that it's already written and has tests that should be passing, it should be included in the PR.

support for file based config

ca2471a

algora-pbc bot mentioned this pull request Apr 18, 2024

Add support for reading command-line options from file(s) #191

Open

algora-pbc bot added the 🙋 Bounty claim label Apr 18, 2024

scalafmt fixed

bb2461b

jdegoes requested changes Apr 19, 2024

View reviewed changes

vivasvan1 added 4 commits April 19, 2024 17:42

build fix set collection-compat to fixed 2.12.0

ea35be5

formatting fixed

f8cba2a

Removing the nio as it doesnt support scalaJS yet

9fbc76d

As of 19th April zio-nio doesnt support scalaJS so using java to read file and zio core for resource management

formatting fixed

79ea5a5

vivasvan1 added 6 commits April 19, 2024 23:57

adding tests init

3fd0278

remove the duplicates

dea5f46

bug-fix: if homedir==workdir so removing dups

07d1823

removing support of configfile arg in JS

952fde7

simplify: removing merging args feature

83a8f6f

removing the merge test as its no longer needed

d655dee

vivasvan1 requested a review from jdegoes April 22, 2024 08:12

vivasvan1 added 2 commits April 24, 2024 14:00

camelCase done

7a6d2cc

fmt fix and removing unneccesary deps

40d0dcf

vivasvan1 added 2 commits June 4, 2024 13:16

full app test case

88489a6

fmt fix + type fix

ba5aa9d

Kalin-Rudnicki reviewed Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vivasvan1 commented Apr 18, 2024 •

edited

Loading

CLAassistant commented Apr 18, 2024 •

edited

Loading

vivasvan1 commented Apr 18, 2024 •

edited

Loading

jdegoes commented Apr 19, 2024

jdegoes left a comment

vivasvan1 commented Apr 19, 2024 •

edited

Loading

vivasvan1 commented May 5, 2024

pablf commented May 6, 2024

vivasvan1 commented Jun 4, 2024 •

edited

Loading

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024 •

edited

Loading

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024 •

edited

Loading

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024

Kalin-Rudnicki Jun 4, 2024 •

edited

Loading

Kalin-Rudnicki Jun 4, 2024 •

edited

Loading

vivasvan1 commented Jun 4, 2024

pablf commented Jun 4, 2024 •

edited

Loading

		content_home: String = "home=true",
		content_cwd: String = "dir=true\nhome=false"

		val cwd = java.lang.System.getProperty("user.dir")
		val homeDirOpt = java.lang.System.getProperty("user.home")

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

Are you sure you want to change the base?

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

Conversation

vivasvan1 commented Apr 18, 2024 • edited Loading

CLAassistant commented Apr 18, 2024 • edited Loading

vivasvan1 commented Apr 18, 2024 • edited Loading

jdegoes commented Apr 19, 2024

jdegoes left a comment

Choose a reason for hiding this comment

vivasvan1 commented Apr 19, 2024 • edited Loading

vivasvan1 commented May 5, 2024

pablf commented May 6, 2024

vivasvan1 commented Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kalin-Rudnicki Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kalin-Rudnicki Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kalin-Rudnicki Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Kalin-Rudnicki Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

vivasvan1 commented Jun 4, 2024

pablf commented Jun 4, 2024 • edited Loading

vivasvan1 commented Apr 18, 2024 •

edited

Loading

CLAassistant commented Apr 18, 2024 •

edited

Loading

vivasvan1 commented Apr 18, 2024 •

edited

Loading

vivasvan1 commented Apr 19, 2024 •

edited

Loading

vivasvan1 commented Jun 4, 2024 •

edited

Loading

Kalin-Rudnicki Jun 4, 2024 •

edited

Loading

Kalin-Rudnicki Jun 4, 2024 •

edited

Loading

Kalin-Rudnicki Jun 4, 2024 •

edited

Loading

Kalin-Rudnicki Jun 4, 2024 •

edited

Loading

pablf commented Jun 4, 2024 •

edited

Loading