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 advancedOption API with arguments support #84

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

RustedBones
Copy link
Contributor

@RustedBones RustedBones commented Jul 18, 2022

As a user, I would like to define the following option in a consistent way: -Xmax-classfile-name 100 with

ScalacOptions.advancedOption(
  "max-classfile-name",
  List("100"),
  version => version.isBetween(V2_12_0, V2_13_0)
)

def other(option: String, args: List[String], isSupported: ScalaVersion => Boolean): ScalacOption =
def other(
option: String,
args: List[String],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also noticing here that the naming is not in line with previously existing privateOption

Copy link
Member

Choose a reason for hiding this comment

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

Hmm do you mean the argument names? This is because def other was added after the ScalacOption breaking changes in 0.4.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the argument name is not the same. It was named arguments in privateOption.
This isn't a problem at all. Just raising an inconsistency :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes it's a good point, I wonder if we could use https://www.scala-lang.org/api/2.13.5/scala/deprecatedName.html to bring them all in line

@RustedBones RustedBones changed the title Add advancedOption API with argument support Add advancedOption API with arguments support Jul 18, 2022
Copy link
Member

@DavidGregory084 DavidGregory084 left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks @RustedBones!

def other(option: String, args: List[String], isSupported: ScalaVersion => Boolean): ScalacOption =
def other(
option: String,
args: 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.

Hmm do you mean the argument names? This is because def other was added after the ScalacOption breaking changes in 0.4.0.

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.

None yet

2 participants