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

improvement: Get rid of more pattern matches in vals #3368

Merged
merged 2 commits into from Oct 10, 2023

Conversation

tgodzik
Copy link
Collaborator

@tgodzik tgodzik commented Oct 9, 2023

I started rewriting the test to have full trees in expect, but that proved to take a long while so instead I created matchSubStructure method and used Chat GPT to help out rewrite it quicker

@tgodzik tgodzik requested a review from kitbellew October 9, 2023 13:57
Copy link
Contributor

@kitbellew kitbellew left a comment

Choose a reason for hiding this comment

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

one cosmetic change, and a few questions.

@@ -93,6 +93,14 @@ class ParseSuite extends TreeSuiteBase with CommonTrees {
)
}

protected def matchSubStructure[T <: Tree](
Copy link
Contributor

Choose a reason for hiding this comment

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

this is elegant! :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

@@ -5,93 +5,104 @@ import scala.meta._
import scala.meta.dialects.Scala211

class LitSuite extends ParseSuite {

// Structure does not always return consisten results
Copy link
Contributor

Choose a reason for hiding this comment

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

consistenT

}

test("-2147483648") {
val Lit(-2147483648) = term("-2147483648")
assertTree(term("-2147483648"))(Lit.Int(-2147483648))
Copy link
Contributor

Choose a reason for hiding this comment

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

this works...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works if printing via .structure is the same. But if it prints differently yet it's a correct value it will be reported as an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously, we would do an unapply which only compares values, so that would be correct. When using Lit.Int(0xCAFEBABE) the int inside will be printed normally, so we would get a mismatch.

}

test("9223372036854775808L") {
intercept[ParseException] { term("9223372036854775808L") }
}

test("9223372036854775807L") {
val Lit(9223372036854775807L) = term("9223372036854775807L")
assertTree(term("9223372036854775807L"))(Lit.Long(9223372036854775807L))
Copy link
Contributor

Choose a reason for hiding this comment

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

and this works...

}

test("0xCAFEBABE") {
val Lit(-889275714) = term("0xCAFEBABE")
val Lit(889275714) = term("-0xCAFEBABE")
assertLiteral(term("0xCAFEBABE"), Lit.Int(-889275714))
Copy link
Contributor

Choose a reason for hiding this comment

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

and this doesn't?

@@ -105,12 +116,15 @@ class LitSuite extends ParseSuite {
}

test("#1382") {
val Lit("\"\"") = term("\"\"\"\"\"\"\"\"")
val Lit("\"\"\"\"\"\"\"") = term("\"\"\"\"\"\"\"\"\"\"\"\"\"")
assertLiteral(term("\"\"\"\"\"\"\"\""), Lit.String("\"\""))
Copy link
Contributor

Choose a reason for hiding this comment

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

and this doesn't either?

@@ -5,93 +5,104 @@ import scala.meta._
import scala.meta.dialects.Scala211

class LitSuite extends ParseSuite {

// Structure does not always return consisten results
private def assertLiteral(obtained: Term, expected: Lit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure what's happening here. why is regular assertTree not working on

assertLiteral(term("0xCAFEBABE"), Lit.Int(-889275714))
assertLiteral(term("42.42"), Lit.Double(42.42))
assertLiteral(term("42.42f"), Lit.Double(42.42f))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main reason is that .structure will print things exactly for parsed terms, but not for those constructed manually. I can change it to the substructure match.

@kitbellew
Copy link
Contributor

... and used Chat GPT to help out rewrite it quicker

i didn't see this 🙂 how does this work? i haven't used it before for code.

@tgodzik
Copy link
Collaborator Author

tgodzik commented Oct 10, 2023

... and used Chat GPT to help out rewrite it quicker

i didn't see this 🙂 how does this work? i haven't used it before for code.

I tried changing the cases to manually construct trees, but that proved a bit tedious, so instead I realized it would be quickes if the change was purely syntactic. In ChatGPT I wrote the expected input (asserts to change) and an example of what I want to turn it to and it managed to change all the cases correctly, so things were much faster this way.

I tried it to also generate full trees, but it's not really good with anything semantic in the code. Syntactic stuff it can do perfectly and regexes seemed rather complex here :s

Copy link
Contributor

@kitbellew kitbellew left a comment

Choose a reason for hiding this comment

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

didn't mean for you to remove the literal assert, was mostly curious.

@kitbellew kitbellew merged commit 53f1fec into scalameta:main Oct 10, 2023
24 checks passed
@tgodzik tgodzik deleted the remove-more-warnings branch October 10, 2023 13:41
@tgodzik
Copy link
Collaborator Author

tgodzik commented Oct 10, 2023

didn't mean for you to remove the literal assert, was mostly curious.

I prefer to have one approach and it changes the tests less.

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