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

fix: avoid escaping unnecessary chars in HTML report suppression regexes #6749

Merged
merged 1 commit into from
Jun 30, 2024

Conversation

chadlwilson
Copy link
Contributor

@chadlwilson chadlwilson commented Jun 30, 2024

Description of Change

Bit of a nitpick: when the HTML report generates an example suppression, it unnecessarily escapes hyphens when constructing a regex inside HTML.

e.g. pkg:maven/org.eclipse.jetty.websocket/websocket-jetty-server

becomes

   <packageUrl regex="true">^pkg:maven/org\.eclipse\.jetty\.websocket/websocket\-jetty\-server@.*$</packageUrl>

This is a little more annoying to read and maintain than it needs to be :-) This would be better:

   <packageUrl regex="true">^pkg:maven/org\.eclipse\.jetty\.websocket/websocket-jetty-server@.*$</packageUrl>

Within regexes, - only needs to be escaped within a character class, and only if not the first character in the character class (ironically the old code relied on this). Since we are not putting elements into character classes and the the relevant [ is escaped we don't need to escape this, and it reads better to not escape.

Similar

  • # has no special meaning in regex, not sure why it was being escaped
  • , has no special meaning outside a repetition qualifier (already {} are escaped), so similarly does not need to be escaped

Technically the escaping is a bit janky right now as it is also escaping some things for the purpose of using JavaScript to construct XML so technically it needs to escapeForXml afterwards, or the packageUrl = pkg:hello-world-</packageUrl> will get you into broken XML trouble, but I guess that is not a major concern :-)

I guess escaping the whitespace escaping with \s is one of these XML-related workarounds based on #2046 so have left it around.

Have test cases been added to cover the new functionality?

no

@boring-cyborg boring-cyborg bot added the core changes to core label Jun 30, 2024
Copy link
Owner

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

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

lgtm

@jeremylong jeremylong merged commit 9b42aed into jeremylong:main Jun 30, 2024
7 checks passed
@chadlwilson chadlwilson deleted the stop-escaping-hyphens branch June 30, 2024 09:36
@jeremylong jeremylong added this to the 10.0 milestone Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core changes to core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants