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.

Sign up for GitHub

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

Allow PluginXmlPatcher to patch multi-line tags #129

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Allow PluginXmlPatcher to patch multi-line tags #129

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1e4c578
fix: allow plugin.xml patching multiline tags
zhutmost May 12, 2024
6745235
test: add PluginXmlPatching tests for multiline/duplicated tags
zhutmost May 13, 2024
5bbc9c7
test: make PluginXmlPatchingTest public
zhutmost May 13, 2024
2b2103b
test: make test more complicated and readable
zhutmost May 17, 2024
aac0469
fix: process comments in pluginXml correctly
zhutmost May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions ideaSupport/src/main/scala/org/jetbrains/sbtidea/xml/PluginXmlPatcher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,18 @@ class PluginXmlPatcher(input: Path, createCopy: Boolean = false) {
content
}

private def tag(str: String, name: String, value: String): String =
if (str.matches(s"(?s)^.*<$name>.+</$name>.*$$"))
str.replaceAll(s"<$name>.+</$name>", s"<$name>$value</$name>")
else {
log.warn(s"$input doesn't have $name tag defined, not patching")
str
private def tag(str: String, name: String, value: String): String = {
val tagPattern = s"(?s)<$name>.*?</$name>".r
val matchCount: Int = tagPattern.findAllIn(str).length
if (matchCount > 0) {
if (matchCount > 1) {
log.warn(s"$input have multiple $name tag defined, only patching the first one")
}
tagPattern.replaceFirstIn(str, s"<$name>$value</$name>")
}
else {
log.warn(s"$input doesn't have $name tag defined, not patching")
str
}
}

}
8 changes: 8 additions & 0 deletions ideaSupport/src/test/resources/org/jetbrains/sbtidea/xml/plugin-duplicatedtags.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<idea-plugin url="https://www.jetbrains.com/idea">
<description>Integrates Volume Snapshot Service W10
This is a very
long description.
It have multiple lines.
</description>
<!-- <description>This is comment</description>-->
</idea-plugin>
7 changes: 7 additions & 0 deletions ideaSupport/src/test/resources/org/jetbrains/sbtidea/xml/plugin-multilinetag.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<idea-plugin url="https://www.jetbrains.com/idea">
<description>Integrates Volume Snapshot Service W10
This is a very
long description.
It have multiple lines.
</description>
</idea-plugin>
43 changes: 36 additions & 7 deletions ideaSupport/src/test/scala/org/jetbrains/sbtidea/xml/PluginXmlPatchingTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ import java.nio.file.{Files, StandardCopyOption}
import scala.collection.JavaConverters.collectionAsScalaIterableConverter
import scala.util.Using

private class PluginXmlPatchingTest extends AnyFunSuite with Matchers with IdeaMock {
class PluginXmlPatchingTest extends AnyFunSuite with Matchers with IdeaMock {

private def getPluginXml = {
private def getPluginXml(pluginXmlName: String = "plugin.xml") = {
val tmpFile = createTempFile("", "plugin.xml")
Using.resource(getClass.getResourceAsStream("plugin.xml")) { stream =>
Using.resource(getClass.getResourceAsStream(pluginXmlName)) { stream =>
Files.copy(stream, tmpFile, StandardCopyOption.REPLACE_EXISTING)
}
tmpFile
Expand All @@ -27,7 +27,7 @@ private class PluginXmlPatchingTest extends AnyFunSuite with Matchers with IdeaM
xml.sinceBuild = "SINCE_BUILD"
xml.untilBuild = "UNTIL_BUILD"
}
val pluginXml = getPluginXml
val pluginXml = getPluginXml()
val initialContent = Files.readAllLines(pluginXml).asScala
val patcher = new PluginXmlPatcher(pluginXml)
val result = patcher.patch(options)
Expand All @@ -49,7 +49,7 @@ private class PluginXmlPatchingTest extends AnyFunSuite with Matchers with IdeaM
xml.pluginDescription = "DESCRIPTION"
xml.sinceBuild = "SINCE_BUILD"
}
val pluginXml = getPluginXml
val pluginXml = getPluginXml()
val initialContent = Files.readAllLines(pluginXml).asScala
val patcher = new PluginXmlPatcher(pluginXml)
val result = patcher.patch(options)
Expand All @@ -71,7 +71,7 @@ private class PluginXmlPatchingTest extends AnyFunSuite with Matchers with IdeaM
xml.pluginDescription = "DESCRIPTION"
xml.untilBuild = "UNTIL_BUILD"
}
val pluginXml = getPluginXml
val pluginXml = getPluginXml()
val initialContent = Files.readAllLines(pluginXml).asScala
val patcher = new PluginXmlPatcher(pluginXml)
val result = patcher.patch(options)
Expand All @@ -88,7 +88,7 @@ private class PluginXmlPatchingTest extends AnyFunSuite with Matchers with IdeaM

test("no modifications done if no patching options are defined") {
val options = pluginXmlOptions { _ => }
val pluginXml = getPluginXml
val pluginXml = getPluginXml()
val initialContent = Files.readAllLines(pluginXml).asScala
val patcher = new PluginXmlPatcher(pluginXml)
val result = patcher.patch(options)
Expand All @@ -99,4 +99,33 @@ private class PluginXmlPatchingTest extends AnyFunSuite with Matchers with IdeaM
diff.size shouldBe 0
}

test("multi-line tags are patched correctly") {
val pluginXml = getPluginXml("plugin-multilinetag.xml")

val options = pluginXmlOptions { xml =>
xml.pluginDescription = "DESCRIPTION\nLINE2"
}
val patcher = new PluginXmlPatcher(pluginXml)
val result = patcher.patch(options)
val newContent = Files.readAllLines(result).asScala

val newContentPattern = s"(?s).*<description>${options.pluginDescription}</description>.*$$".r
newContent.mkString("\n") should fullyMatch regex newContentPattern
Copy link
Member

Choose a reason for hiding this comment

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

Using regular expressions in tests over-complicates it. It will be harder to read failed test output and maintain the test.
Let's use a simple equality test (shouldEqual/shouldBe?)

}

test("duplicate tags are only patched the first one") {
Copy link
Member

Choose a reason for hiding this comment

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

One of the issues you tried to fix in org.jetbrains.sbtidea.xml.PluginXmlPatcher#tag is to ignore comments.
(due to the description in PR comments)

However, it doesn't do it.
If you go to plugin-duplicatedtags.xml and move the commented description to the top, the patcher will edit the commented description.

What's more, this test would pass and not detect the bug.
But if it used simple shouldEqual/shouldBe it would fail and we would notice the issue

Copy link
Member

Choose a reason for hiding this comment

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

After fixing this, I would add two comments—before and after the main description tag

Copy link
Member

Choose a reason for hiding this comment

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

To address it, we could first find all the ranges of the comments using something like

import scala.util.matching.Regex

val CommentPattern = new Regex("(?s)<!--.*?-->")

def findCommentRanges(xml: String): Seq[Range] = 
  CommentPattern.findAllMatchIn(xml).map(m => m.start until m.end).toList

and then ignore those pattern matches which are inside those ranges

Copy link
Author

Choose a reason for hiding this comment

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

OK. I will fix them. Just a few days (~1 week), because I've got something else going on.

val pluginXml = getPluginXml("plugin-duplicatedtags.xml")

val options = pluginXmlOptions { xml =>
xml.pluginDescription = "DESCRIPTION\nLINE2"
}
val patcher = new PluginXmlPatcher(pluginXml)
val result = patcher.patch(options)
val newContent = Files.readAllLines(result).asScala

val pattern1 = s"(?s)<description>.*?</description>".r
pattern1.findAllMatchIn(newContent.mkString("\n")).size shouldBe 2
val pattern2 = s"(?s).*<description>${options.pluginDescription}</description>.*<description>.*</description>.*$$".r
newContent.mkString("\n") should fullyMatch regex pattern2
}
}