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

Add support for custom option formats #186

Merged

Conversation

regadas
Copy link
Collaborator

@regadas regadas commented Mar 9, 2020

Hi @alexarchambault I still need to fix docs etc but wanted to know you think about this.
Also not sure about the naming ¯_(ツ)_/¯

Related to #181

@alexarchambault
Copy link
Owner

Thanks a lot @regadas!

That looks fine overall, I'll review in more detail tomorrow or later during the week at the latest.

@alexarchambault
Copy link
Owner

OptionFormatter is fine by me as a name.

Copy link
Owner

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

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

Thanks for that! I added some comments.

Also, if you want this to work when extending CaseApp, that is when using case-app like

object MyApp extends CaseApp[MyOptions] {
  def run(options: MyOptions, args: RemainingArgs): Unit = ???
}

you need some more changes.

#176 illustrates what needs to be added for the extra field in Parser (here, defaultNameFormatter) to be propagated through all Parser instances (likely, some override def defaultNameFormatter = underlying.defaultNameFormatter here or there).

At the very end of the changes of #176, it also adds a test case for that kind of use.

Copy link
Collaborator Author

@regadas regadas left a comment

Choose a reason for hiding this comment

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

Hi @alexarchambault thanks for the review! (Sorry about the re-format some of them still went through).

Yes supporting the case when extending CaseApp will be good 👍

@regadas regadas marked this pull request as ready for review March 13, 2020 19:14
Copy link
Owner

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

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

Thanks! I added one last comment.

@regadas
Copy link
Collaborator Author

regadas commented Mar 14, 2020

@alexarchambault Thanks for taking a look at this! 👍

@regadas
Copy link
Collaborator Author

regadas commented Mar 14, 2020

@alexarchambault btw would you consider adding scalafmt? 😄

@alexarchambault
Copy link
Owner

It could be added. Especially now that scalameta/scalafmt#1362 should be fixed, it may possible not to add it while not messing too much with the current manual formatting.

@alexarchambault
Copy link
Owner

So merging this, thanks again!

@alexarchambault alexarchambault merged commit 28899aa into alexarchambault:master Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants