Try to parse all valid Testkit HOCON comments #1832
Conversation
mdedetrich
force-pushed
the
parse-all-valid-hocon-testkit-comments
branch
2 times, most recently
from
690e93c
to
27a5621
Compare
There was a problem hiding this comment.
Thanks for the PR!
For testing, I would suggest an integration test, simply copy/pasting https://github.com/scalacenter/scalafix/blob/main/scalafix-tests/input/src/main/scala-2/test/LintAsserts.scala (the Disable rule is useless though) in its current directory, and adding a license header.
scalafix-testkit/src/main/scala/scalafix/testkit/RuleTest.scala
Outdated
Show resolved
Hide resolved
| val rulesConf = ConfGet | ||
| .getKey(conf, "rules" :: "rule" :: Nil) | ||
| .getOrElse(Conf.Lst(Nil)) |
There was a problem hiding this comment.
per the message in the exception, this should be lifted up to the lazilyParsedComments closure to effectively skip comments that are HOCON but do not contain rule or rules
There was a problem hiding this comment.
So I thought the failure in parsing is done in Conf.parseString(test.testName, syntax) (which is why I pattern match against it). The idea behind my solution is that we create a lazy collection called lazilyParsedComments of successfully/unsuccessfully parsed HOCON comments and then we do collectFirst to get the first successful parsing (rather than currently where we just assume the first comment we get via SemanticRuleSuite.findTestkitComment is a correct comment and it just happens to fail because while its a Scala source comment, its not a valid HOCON.
In other words, the logic of figuring out what is a correct HOCON comment has already happened by the time we get to rulesConf
There was a problem hiding this comment.
to get the first successful parsing
This is definitely an improvement and should cover your original use-case of a long comment. The implementation was "the first comment should be a HOCON with a rule/rules defined", and is now "the first HOCON-parsable comment should have a rule/rules defined".
I was thinking about the case where the first comment is accidentally a valid HOCON snippet, but does not contain a rule or rules attribute, and advocating for the implementation of "there should be one HOCON-parsable comment with a rule/rules defined". Feel free to ignore that use-case.
There was a problem hiding this comment.
This is definitely an improvement and should cover your original use-case of a long comment
Actually, it does not - the test I added in fd35cf2 demonstrates that. I will try to do what I was suggesting in another commit and see if it helps.
scalafix-testkit/src/main/scala/scalafix/testkit/RuleTest.scala
Outdated
Show resolved
Hide resolved
scalafix-testkit/src/main/scala/scalafix/testkit/RuleTest.scala
Outdated
Show resolved
Hide resolved
mdedetrich
force-pushed
the
parse-all-valid-hocon-testkit-comments
branch
from
27a5621
to
079f867
Compare
|
@bjaglin Thanks for reviewing the PR, I addressed some of the comments but I may have an incorrect understanding of what the problem is since it appears that |
mdedetrich
force-pushed
the
parse-all-valid-hocon-testkit-comments
branch
2 times, most recently
from
144d81d
to
b6ec3a9
Compare
|
@bjaglin Okay so I have done some improvements to the PR, on my local machine Would it be possible to re-run the CI just to confirm I didn't create any regressions? Once that is confirmed I can add a test to confirm that we solve the issue and/or discuss the solution (I think there is a misunderstanding here which I can also clarify with comments). |
mdedetrich
force-pushed
the
parse-all-valid-hocon-testkit-comments
branch
from
b6ec3a9
to
96f8d00
Compare
Done. I also changed the default, so I shouldn't need to approve execution at next push 🤞 |
| val rulesConf = ConfGet | ||
| .getKey(conf, "rules" :: "rule" :: Nil) | ||
| .getOrElse(Conf.Lst(Nil)) |
There was a problem hiding this comment.
to get the first successful parsing
This is definitely an improvement and should cover your original use-case of a long comment. The implementation was "the first comment should be a HOCON with a rule/rules defined", and is now "the first HOCON-parsable comment should have a rule/rules defined".
I was thinking about the case where the first comment is accidentally a valid HOCON snippet, but does not contain a rule or rules attribute, and advocating for the implementation of "there should be one HOCON-parsable comment with a rule/rules defined". Feel free to ignore that use-case.
scalafix-testkit/src/main/scala/scalafix/testkit/RuleTest.scala
Outdated
Show resolved
Hide resolved
|
@bjaglin Thanks for helping out, I will try and have a look at why the test is failing in the next few days |
bjaglin
force-pushed
the
parse-all-valid-hocon-testkit-comments
branch
2 times, most recently
from
f564944
to
ae881f8
Compare
bjaglin
force-pushed
the
parse-all-valid-hocon-testkit-comments
branch
from
ae881f8
to
44aff59
Compare
scalafix-testkit/src/main/scala/scalafix/testkit/RuleTest.scala
Outdated
Show resolved
Hide resolved
|
@bjaglin Just pushed the fix I was talking about, tests are passing locally so all should be good. |
mdedetrich
force-pushed
the
parse-all-valid-hocon-testkit-comments
branch
from
81dac96
to
a92693d
Compare
|
Thanks @mdedetrich ! |
Resolves: #1828
Pinging @bjaglin since you commented on the original issue. The only thing that is missing is a test, its my first time working on scalafix so not entirely sure where to put such a test and what type of test.