Allow PluginXmlPatcher to patch multi-line tags#129
Open
zhutmost wants to merge 5 commits intoJetBrains:masterfrom
Open
Allow PluginXmlPatcher to patch multi-line tags#129zhutmost wants to merge 5 commits intoJetBrains:masterfrom
zhutmost wants to merge 5 commits intoJetBrains:masterfrom
Conversation
Author
|
And I also check whether multiple tags with a same name are defined. If so, a warning will be printed and only the first one will be replaced. That is because sometime I write comments in plugin.xml, the original version will patch all of them like this: <description>
determined by build.sbt
</description>
<!-- <description>
This is commented out.
</description> -->
...will be changed to: <description>
replacer
</description> -->
...That is not what I want. |
Member
|
Thanks for the PR! |
Author
|
@unkarjedy OK. but there is a small issue that the current Looks that it is unrelated to my PR |
Author
|
Forgive my last comment. (I dont know what happened. After multi-runs, it looks good now.) I have added pluginXmlPatching tests for multiple tags and duplicated tags. Here is my all sbt test log: |
Member
Thanks for noticing this! |
Author
|
OK, done. Looks good. You can consider merging now. |
unkarjedy
requested changes
May 15, 2024
| val newContent = Files.readAllLines(result).asScala | ||
|
|
||
| val newContentPattern = s"(?s).*<description>${options.pluginDescription}</description>.*$$".r | ||
| newContent.mkString("\n") should fullyMatch regex newContentPattern |
Member
There was a problem hiding this comment.
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?)
| newContent.mkString("\n") should fullyMatch regex newContentPattern | ||
| } | ||
|
|
||
| test("duplicate tags are only patched the first one") { |
Member
There was a problem hiding this comment.
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
Member
There was a problem hiding this comment.
After fixing this, I would add two comments—before and after the main description tag
Member
There was a problem hiding this comment.
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).toListand then ignore those pattern matches which are inside those ranges
Author
There was a problem hiding this comment.
OK. I will fix them. Just a few days (~1 week), because I've got something else going on.
Author
|
@unkarjedy I have fixed your two issues. please review. I check all the comments and , pairs, and only replace non-comment ones. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If my plugin.xml have a multi-line tag (i.e., includes '\n') like this:
the current PluginXmlPatcher cannot patch it correctly.
The current code is:
Detailed reason analysis:
The regex in the if-condition with "(?s)" enables DOTALL mode, so it can match the string with '\n'. And, the regex in the
replacestatement does not have a "(?s)", so it cannot match multiline tags, and nothing will be replaced.