-
Notifications
You must be signed in to change notification settings - Fork 30
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
bug: false positives on markdown #40
base: main
Are you sure you want to change the base?
Conversation
…nstead of html, at the cost of a very unreadable regular expression. This matches the following header: ([![Community Plus header](https://github.com/newrelic/opensource-website/raw/main/src/images/categories/Community_Plus.png)](https://opensource.newrelic.com/oss-category/#community-plus) In addition to the prior header: <a href="https://opensource.newrelic.com/oss-category/#community-plus"><picture><source media="(prefers-color-scheme: dark)" srcset="https://github.com/newrelic/opensource-website/raw/main/src/images/categories/dark/Community_Plus.png"><source media="(prefers-color-scheme: light)" srcset="https://github.com/newrelic/opensource-website/raw/main/src/images/categories/Community_Plus.png"><img alt="New Relic Open Source community plus project banner." src="https://github.com/newrelic/opensource-website/raw/main/src/images/categories/Community_Plus.png"></picture></a> Apologies to future devs who have to strain their eyes to read the regular expression. It is fundamentally (A)|(B), where the contents of A and B are escaped.
ef9f302
to
94e34cc
Compare
Ok, so I looked around a bit to refresh how this was supposed to work. For reference, the rules for repolinter are defined here: https://github.com/newrelic-forks/repolinter/blob/main/docs/rules.md Some context#38 had the assertion:
However, that's not the case. The header used to be a simple markdown-formatted image, and instead of making the regex a single line (and dealing with the complicated escaping), the
And the ruleset used to have these checks:
How we got to the current state#34 is where I made the update to use the dynamic light/dark mode banners. However, I should've removed the 2nd line in the pattern as it's redundant and is contained in the first pattern. Essentially, the 2nd pattern line makes no difference, as highlighted in the image below: The actual issueThe 1st line (and only the 1st line) is really what we want everyone to update their README to use, because it supports both light/dark mode images. Otherwise, you get the old image that looks like this with the dark mode (as seen on https://github.com/newrelic/newrelic-maui-plugin): The correct image has colors inverted to work in dark mode, like so: For correctness, the fix we should employ is to update every ruleset and remove that 2nd line, since it's just causing confusion. And repo maintainers should update their READMEs to use the latest snippet found at https://github.com/newrelic/opensource-website/wiki/Open-Source-Category-Snippets. In fact, that's why the |
I have pushed a new version of the pull request that implements this approach. However, I'm not sure how to test it, @jbeveland27. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
About testing: it's a bit complicated. You need to point a repolinter config_url
at your branch in this repo. And then you can kick off the repolinter action manually with a workflow_dispatch
and see if it does what you expect. Kind of a pain to set up by hand. I usually fork a repo, modify to simulate what I'm testing, and then run the repolinter workflow a few way to see if it behaves accordingly.
For a change like this, I'm reasonably confident in just saying this is good and we can merge.
Fix false positives against catalog headers expressed with markdown instead of html.
Fixes:
Apologies to future devs who have to strain their eyes to read the regular expression. It is fundamentally (A)|(B), where the contents of A and B are escaped.
This matches the following header:
([![Community Plus header](https://github.com/newrelic/opensource-website/raw/main/src/images/categories/Community_Plus.png)](https://opensource.newrelic.com/oss-category/#community-plus)
In addition to the prior header:
<a href="https://opensource.newrelic.com/oss-category/#community-plus"><picture><source media="(prefers-color-scheme: dark)" srcset="https://github.com/newrelic/opensource-website/raw/main/src/images/categories/dark/Community_Plus.png"><source media="(prefers-color-scheme: light)" srcset="https://github.com/newrelic/opensource-website/raw/main/src/images/categories/Community_Plus.png"><img alt="New Relic Open Source community plus project banner." src="https://github.com/newrelic/opensource-website/raw/main/src/images/categories/Community_Plus.png"></picture></a>