-
Notifications
You must be signed in to change notification settings - Fork 326
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
Specify cursor position when creating new files #1546
Conversation
import scala.meta.internal.metals.NewFileTemplate | ||
import org.scalacheck.Gen | ||
import org.scalacheck.Prop.forAll | ||
|
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.
/cc @olafurpg dogfooding the new scalacheck module 👇
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.
Do we really need Scalacheck here though? It's supposed to be used over a set of possible values and here it is just normal testing. I have never found a usage for property based testing and maybe it would be better to keep the tests consistent?
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.
It's supposed to be used over a set of possible values and here it is just normal testing
I'm using Scalacheck to test "all" possible positions of the cursor inside a (fixed) document.
Do we really need Scalacheck here though?
It's not strictly necessary, but it just gives me a little more confidence that the computation is not right by chance but it holds for "all" possible cursor positions.
That said, if you prefer we can remove it.
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.
I'm using Scalacheck to test "all" possible positions of the cursor inside a (fixed) document.
I am totally blind and have not seen it. It's a perfectly valid scenario to use it in. 👍
2feb190
to
50bc72b
Compare
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.
Thanks for adding this feature! It will be much more comfortable to create new files now.
I am not sure about using Scalacheck here though.
|
||
class NewFileTemplateSuite extends BaseSuite with ScalaCheckSuite { | ||
|
||
test("it requires exactly one cursor marker") { |
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.
Should we stick to short names as in other suites?
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.
Ah, absolutely, I completely forgot. Fixed
import scala.meta.internal.metals.NewFileTemplate | ||
import org.scalacheck.Gen | ||
import org.scalacheck.Prop.forAll | ||
|
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.
Do we really need Scalacheck here though? It's supposed to be used over a set of possible values and here it is just normal testing. I have never found a usage for property based testing and maybe it would be better to keep the tests consistent?
Unrelated flaky test (we should probably mark it as Merging |
|case class Foo() | ||
|""".stripMargin | ||
val cursorOffsetGen = Gen.chooseNum(0, template.length) | ||
forAll(cursorOffsetGen) { cursorOffset => |
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.
What is the difference between forAll
here and 0.to(template.length).foreach { .. }
?
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.
ah, good point: in this case just the number of iterations (100 with ScalaCheck). I can revert to just using a range
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.
I think 1.to(100)
would be clearer and more consistent with the rest of the tests. Property based testing is more helpful when you can't tests all possible inputs.
Closes #1529
Previously we would set the cursor position at the beginning of the file when creating a new file. Now we place the cursor in a more convenient position, depending on the type of file.
Demo: