-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Multivalued setting can be set multiply #9731
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
Changes from 4 commits
3e24960
973b285
196b3fa
ae55323
22e76e8
0772585
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,15 @@ | ||
package dotty.tools.dotc | ||
package config | ||
|
||
import collection.mutable.ArrayBuffer | ||
import scala.util.{ Success, Failure } | ||
import reflect.ClassTag | ||
import core.Contexts._ | ||
import scala.annotation.tailrec | ||
import dotty.tools.io.{ AbstractFile, Directory, JarArchive, PlainDirectory } | ||
|
||
// import annotation.unchecked | ||
// Dotty deviation: Imports take precedence over definitions in enclosing package | ||
// (Note that @unchecked is in scala, not annotation, so annotation.unchecked gives | ||
// us a package, which is not what was intended anyway). | ||
import dotty.tools.io.{AbstractFile, Directory, JarArchive, PlainDirectory} | ||
|
||
import annotation.tailrec | ||
import collection.mutable.ArrayBuffer | ||
import language.existentials | ||
import reflect.ClassTag | ||
import scala.util.{Success, Failure} | ||
|
||
object Settings { | ||
|
||
|
@@ -25,7 +22,7 @@ object Settings { | |
val OutputTag: ClassTag[AbstractFile] = ClassTag(classOf[AbstractFile]) | ||
|
||
class SettingsState(initialValues: Seq[Any]) { | ||
private var values = ArrayBuffer(initialValues: _*) | ||
private val values = ArrayBuffer(initialValues: _*) | ||
private var _wasRead: Boolean = false | ||
|
||
override def toString: String = s"SettingsState(values: ${values.toList})" | ||
|
@@ -91,38 +88,45 @@ object Settings { | |
|
||
def isDefaultIn(state: SettingsState): Boolean = valueIn(state) == default | ||
|
||
def isMultivalue: Boolean = implicitly[ClassTag[T]] == ListTag | ||
|
||
def legalChoices: String = | ||
if (choices.isEmpty) "" | ||
else choices match { | ||
case r: Range => s"${r.head}..${r.last}" | ||
case xs: List[?] => xs.mkString(", ") | ||
choices match { | ||
case xs if xs.isEmpty => "" | ||
case r: Range => s"${r.head}..${r.last}" | ||
case xs: List[?] => xs.toString | ||
} | ||
|
||
def isLegal(arg: Any): Boolean = | ||
if (choices.isEmpty) | ||
arg match { | ||
case _: T => true | ||
case _ => false | ||
} | ||
else choices match { | ||
choices match { | ||
case xs if xs.isEmpty => | ||
arg match { | ||
case _: T => true | ||
case _ => false | ||
} | ||
case r: Range => | ||
arg match { | ||
case x: Int => r.head <= x && x <= r.last | ||
case _ => false | ||
} | ||
case xs: List[?] => | ||
xs contains arg | ||
xs.contains(arg) | ||
} | ||
|
||
def tryToSet(state: ArgsSummary): ArgsSummary = { | ||
val ArgsSummary(sstate, arg :: args, errors, warnings) = state | ||
def update(value: Any, args: List[String]) = | ||
if (changed) | ||
ArgsSummary(updateIn(sstate, value), args, errors, warnings :+ s"Flag $name set repeatedly") | ||
else { | ||
changed = true | ||
ArgsSummary(updateIn(sstate, value), args, errors, warnings) | ||
} | ||
val (value1, twicely) = | ||
if changed && isMultivalue then | ||
val value0 = value.asInstanceOf[List[String]] | ||
val current = valueIn(sstate).asInstanceOf[List[String]] | ||
val newly = current ++ value0.filterNot(current.contains) | ||
(newly, value0.exists(current.contains)) | ||
else | ||
(value, changed) | ||
val dangers = if twicely then warnings :+ s"Flag $name set repeatedly" else warnings | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This warning is not informative for multivalued settings, as it's OK to set them multiple times. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I wasn't clear. I don't mean to suppress the warning. I mean to improve the warning to say a specific value is set multiple times. |
||
changed = true | ||
ArgsSummary(updateIn(sstate, value1), args, errors, dangers) | ||
def fail(msg: String, args: List[String]) = | ||
ArgsSummary(sstate, args, errors :+ msg, warnings) | ||
def missingArg = | ||
|
@@ -229,7 +233,7 @@ object Settings { | |
* | ||
* to get their arguments. | ||
*/ | ||
protected def processArguments(state: ArgsSummary, processAll: Boolean, skipped: List[String]): ArgsSummary = { | ||
protected[config] def processArguments(state: ArgsSummary, processAll: Boolean, skipped: List[String]): ArgsSummary = { | ||
def stateWithArgs(args: List[String]) = ArgsSummary(state.sstate, args, state.errors, state.warnings) | ||
state.arguments match { | ||
case Nil => | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ import Diagnostic._ | |
* - The reporter is not flushed and the message containers capture a | ||
* `Context` (about 4MB) | ||
*/ | ||
class StoreReporter(outer: Reporter) extends Reporter { | ||
class StoreReporter(outer: Reporter = null) extends Reporter { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest remove this default value, as it is not common for |
||
|
||
protected var infos: mutable.ListBuffer[Diagnostic] = null | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,23 +1,23 @@ | ||||||
package dotty.tools | ||||||
package dotc | ||||||
|
||||||
import reporting.StoreReporter | ||||||
import vulpix.TestConfiguration | ||||||
|
||||||
import org.junit.Test | ||||||
import org.junit.Assert._ | ||||||
import dotty.tools.vulpix.TestConfiguration.mkClasspath | ||||||
|
||||||
import java.nio.file._ | ||||||
|
||||||
import dotty.tools.vulpix.TestConfiguration.mkClasspath | ||||||
import org.junit.Test | ||||||
import org.junit.Assert._ | ||||||
|
||||||
class SettingsTests { | ||||||
|
||||||
@Test def missingOutputDir: Unit = { | ||||||
@Test def missingOutputDir: Unit = | ||||||
val options = Array("-d", "not_here") | ||||||
val reporter = Main.process(options) | ||||||
val reporter = Main.process(options, reporter = StoreReporter()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
assertEquals(1, reporter.errorCount) | ||||||
assertEquals("'not_here' does not exist or is not a directory or .jar file", reporter.allErrors.head.message) | ||||||
} | ||||||
|
||||||
@Test def jarOutput: Unit = { | ||||||
val source = "tests/pos/Foo.scala" | ||||||
|
@@ -29,13 +29,12 @@ class SettingsTests { | |||||
assertTrue(Files.exists(out)) | ||||||
} | ||||||
|
||||||
@Test def t8124: Unit = { | ||||||
val source = Paths.get("tests/pos/Foo.scala").normalize | ||||||
@Test def t8124: Unit = | ||||||
val source = Paths.get("tests/pos/Foo.scala").normalize | ||||||
val outputDir = Paths.get("out/testSettings").normalize | ||||||
if (Files.notExists(outputDir)) Files.createDirectory(outputDir) | ||||||
val options = Array("-encoding", "-d", outputDir.toString, source.toString) | ||||||
val reporter = Main.process(options) | ||||||
if Files.notExists(outputDir) | ||||||
Files.createDirectory(outputDir) | ||||||
val options = Array("-encoding", "-d", outputDir.toString, source.toString) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The argument is missing for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ticket is about the missing arg. |
||||||
val reporter = Main.process(options, reporter = StoreReporter()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
assertEquals(1, reporter.errorCount) | ||||||
} | ||||||
|
||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
package dotty.tools.dotc | ||
package config | ||
|
||
import CommandLineParser.tokenize | ||
import Settings._ | ||
|
||
import org.junit.Test | ||
import org.junit.Assert._ | ||
|
||
class ScalaSettingsTests: | ||
|
||
@Test def `A multistring setting is multivalued`: Unit = | ||
class SUT extends SettingGroup: | ||
val language: Setting[List[String]] = MultiStringSetting("-language", "feature", "Enable one or more language features.") | ||
val sut = SUT() | ||
val args = tokenize("-language:implicitConversions,dynamics") | ||
val sumy = ArgsSummary(sut.defaultState, args, errors = Nil, warnings = Nil) | ||
val res = sut.processArguments(sumy, processAll = true, skipped = Nil) | ||
val set = sut.language.valueIn(res.sstate) | ||
assertEquals(1, args.length) | ||
assertTrue("No warnings!", res.warnings.isEmpty) | ||
assertTrue("No errors!", res.errors.isEmpty) | ||
assertTrue("Has the feature", set.contains("implicitConversions")) | ||
assertTrue("Has the feature", set.contains("dynamics")) | ||
|
||
@Test def `t9719 Apply -language more than once`: Unit = | ||
class SUT extends SettingGroup: | ||
val language: Setting[List[String]] = MultiStringSetting("-language", "feature", "Enable one or more language features.") | ||
val sut = SUT() | ||
val args = tokenize("-language:implicitConversions -language:dynamics") | ||
val sumy = ArgsSummary(sut.defaultState, args, errors = Nil, warnings = Nil) | ||
val res = sut.processArguments(sumy, processAll = true, skipped = Nil) | ||
val set = sut.language.valueIn(res.sstate) | ||
assertEquals(2, args.length) | ||
assertTrue("No warnings!", res.warnings.isEmpty) | ||
assertTrue("No errors!", res.errors.isEmpty) | ||
assertTrue("Has the feature", set.contains("implicitConversions")) | ||
assertTrue("Has the feature", set.contains("dynamics")) | ||
|
||
@Test def `Warn if multistring element is supplied multiply`: Unit = | ||
class SUT extends SettingGroup: | ||
val language: Setting[List[String]] = MultiStringSetting("-language", "feature", "Enable one or more language features.") | ||
val sut = SUT() | ||
val args = tokenize("-language:dynamics -language:implicitConversions -language:dynamics") | ||
val sumy = ArgsSummary(sut.defaultState, args, errors = Nil, warnings = Nil) | ||
val res = sut.processArguments(sumy, processAll = true, skipped = Nil) | ||
val set = sut.language.valueIn(res.sstate) | ||
assertEquals(3, args.length) | ||
assertEquals("Must warn", 1, res.warnings.length) | ||
assertTrue("No errors!", res.errors.isEmpty) | ||
assertTrue("Has the feature", set.contains("implicitConversions")) | ||
assertTrue("Has the feature", set.contains("dynamics")) | ||
|
||
end ScalaSettingsTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be simpler:
newly.size <= current.size
?