Refactorings might mess with parens #94
Conversation
Prior versions of `CommonPrintUtils.balanceParens` treated parens in string constants just like parens in regular Scala code which resulted in bugs like ticket #1002088. This commit improves the aforementioned function, that also played a prominent role in ticket #1002166, by delegating counting parens to a helper function, that handles comments and string constants properly. Fixes #1002088
The term "brackets" is used for "<>", "()", "{}" and "[]", while parens usually
refers to "()" and "{}" only. As the function in question accepts the brackets
as parameters, "brackets" is more appropriate than "parens".
|
Refer to this link for build results (access rights to CI server needed): https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/100/ |
| @@ -40,7 +40,7 @@ trait Indentations { | |||
| if(memoizedSourceWithoutComments contains tree.pos.source.path) { | |||
| memoizedSourceWithoutComments(tree.pos.source.path) | |||
| } else { | |||
| val src = CommentsUtils.stripComment(tree.pos.source.content) | |||
| val src = SourceUtils.stripComment(tree.pos.source.content) | |||
There was a problem hiding this comment.
This rename breaks the IDE and maybe other clients.
|
Beside from the source incompatibility this code looks good (I must admit I didn't verify the correctness of this imperative monster, I hope you don't expect that ;)). |
|
First, let me apolozige for that piece of code.. I have no memory of why this was introduced, but I've just deleted that part and no test fails: So I think it's safe to just delete it. |
|
Otherwise, looks good! But I agree with Simon that we should try to keep the source compatible. |
|
Refer to this link for build results (access rights to CI server needed): https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/101/ |
`CommonPrintUtils.balanceBrackets` used to contain some code that was meant to add missing closing brackets directly after the last closing bracket, instead of just appending them to the layout. Given the fact that the code was broken anyway, (it didn't consider comments, string constants, etc.), all tests pass without it, and the original author thinks that it's not needed, it seems safe to remove it.
|
Thanks for the review! I've just reintroduced As for that mysterious piece of code: I've just removed at, as @misto suggested. Maybe we should wait a few days before merging, so that I can see if I experience any oddities. |
|
Refer to this link for build results (access rights to CI server needed): https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/102/ |
|
I did some digging ( |
|
Thanks for the research; the code has been deleted with 6279d92. |
|
Did I just forget to merge this, or were there any issues left? :-) |
|
I think everything is resolved. LGTM |
…nthesis-1002088 Refactorings might mess with parens
mlangc
deleted the
organize-imports-adds-opening-parenthesis-1002088
branch
This PR, which is closely related to #93, deals mostly with
CommonPrintUtils.balanceBracketsonce more (details can be found in the commit messages). Please note that I still don't trust this function completely, as I don't understand what the"\\"in(see https://github.com/mlangc/scala-refactoring/blob/organize-imports-adds-opening-parenthesis-1002088/src/main/scala/scala/tools/refactoring/sourcegen/CommonPrintUtils.scala#L61) is about. @misto Maybe you can clarify?
Last but not least, these changes fix Ticket #1002088.