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

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

Closed
wants to merge 3 commits into from

Conversation

vivasvan1
Copy link

/claim #191

closes #191

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2023

CLA assistant check
All committers have signed the CLA.

@vivasvan1
Copy link
Author

I have not changed any file in zio-cli-docs yet it throws this error. should i try to fix it??

[info] [webpackbar] Compiling Client
[info] [webpackbar] Compiling Server
[success] [webpackbar] Client: Compiled with some errors in 27.48s

Error:  Client bundle compiled with errors therefore further build is impossible.
Module parse failed: Unexpected token (961:17)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
|       'special-attr': [],
|       'attr-value': {
>         pattern: /=\s*(?:"[^"]*"|'[^']*'|[^\s'">=]+)/,
|         inside: {
|           'punctuation': [{
[error] java.lang.RuntimeException: Failed to build the website!
[error] 	at scala.sys.package$.error(package.scala:30)
[error] 	at zio.sbt.WebsitePlugin$.exit(WebsitePlugin.scala:135)
[error] 	at zio.sbt.WebsitePlugin$.$anonfun$buildWebsiteTask$1(WebsitePlugin.scala:[204](https://github.com/zio/zio-cli/actions/runs/6658650685/job/18095973316?pr=278#step:7:205))
[error] 	at zio.sbt.WebsitePlugin$.$anonfun$buildWebsiteTask$1$adapted(WebsitePlugin.scala:200)
[error] 	at scala.Function1.$anonfun$compose$1(Function1.scala:49)
[error] 	at sbt.internal.util.$tilde$greater.$anonfun$$u2219$1(TypeFunctions.scala:63)
[error] 	at sbt.std.Transform$$anon$4.work(Transform.scala:69)
[error] 	at sbt.Execute.$anonfun$submit$2(Execute.scala:283)
[error] 	at sbt.internal.util.ErrorHandling$.wideConvert(ErrorHandling.scala:24)
[error] 	at sbt.Execute.work(Execute.scala:292)
[error] 	at sbt.Execute.$anonfun$submit$1(Execute.scala:283)
[error] 	at sbt.ConcurrentRestrictions$$anon$4.$anonfun$submitValid$1(ConcurrentRestrictions.scala:265)
[error] 	at sbt.CompletionService$$anon$2.call(CompletionService.scala:65)
[error] 	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
[error] 	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
[error] 	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
[error] 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
[error] 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
[error] 	at java.base/java.lang.Thread.run(Thread.java:840)
[error] (docs / buildWebsite) Failed to build the website!

self.command
.parse(prefix(self.command) ++ args, self.config)
.parse(prefix(self.command) ++ arg_from_config_files ++ args, self.config)
Copy link
Member

Choose a reason for hiding this comment

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

Hi! I don't think including arg_from_config_files there would work. It's probably best to do this inside command.parse so you can have more control in how args are parsed.

Copy link
Author

Choose a reason for hiding this comment

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

ok move it inside command.parse

@@ -56,6 +57,75 @@ object CliApp {
def printDocs(helpDoc: HelpDoc): UIO[Unit] =
printLine(helpDoc.toPlaintext(80)).!

def checkAndGetOptionsFilePaths(topLevelCommand: String): List[String] = {
Copy link
Member

Choose a reason for hiding this comment

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

This is side-effecting, so should return Task[List[String]] for consistency with the rest of ZIO ecosystem.

// Read the contents of the files at the given file paths
val options: List[String] = filePaths.flatMap { filePath =>
val source = scala.io.Source.fromFile(filePath)
source.getLines().toList
Copy link
Member

Choose a reason for hiding this comment

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

This creates a leak because the source is never closed. Also, it's an inefficient way to read a file. Better to use something like ZIO.readFile.

Copy link
Member

@jdegoes jdegoes left a comment

Choose a reason for hiding this comment

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

A few minor changes, then good to merge!

@vivasvan1
Copy link
Author

@jdegoes thanks for the comment. I will update and commit changes soon.

@jdegoes
Copy link
Member

jdegoes commented Jan 16, 2024

@vivasvan1 Please do try to fix any CI error. 🙏

@jdegoes
Copy link
Member

jdegoes commented Jan 16, 2024

@vivasvan1 Please re-open if you get a chance to fix conflicts! 🙏

@@ -115,8 +185,9 @@ object CliApp {
case Command.Subcommands(parent, _) => prefix(parent)
}

val arg_from_config_files = loadOptionsFromFile(self.name)
Copy link
Member

Choose a reason for hiding this comment

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

camelCase please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for reading command-line options from file(s)
4 participants