add numeric icon feature #762
Conversation
|
This will also be useful for the ESRI project, thanks @dkuntze! |
|
Running into same issue w/ Jet2. This successfully gets converted into an icon: This does not: Both should work. Checked the proposed regex, worked as expected. |
|
here some false positives that were found scanning the content:
|
|
I'm starting to think this is an impossible task. I may just document what works and what doesn't and call it a day. |
|
This PR will trigger no release when merged. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #762 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 45 46 +1
Lines 3641 3979 +338
==========================================
+ Hits 3641 3979 +338 ☔ View full report in Codecov by Sentry. |
|
@tripodsan I gave it another go and added your false positives to the tests |
src/steps/rewrite-icons.js
Outdated
|
|
||
| // Skip if this looks like part of a pattern | ||
| if (/[\d:T]/.test(beforeChar) || /[\d:]/.test(afterChar)) { | ||
| return idx + 1; |
There was a problem hiding this comment.
what about the rest of the text? eg 00:00:00 and :icon: ?
There was a problem hiding this comment.
should :icon: be ignored?
There was a problem hiding this comment.
I added tests for :icon: to work (literal) and 00:00:00 to fail.
Both are working.
There was a problem hiding this comment.
I mean, the return here aborts the for loop over all matches. so for the pattern :foo:2 and :icon: the icon is not replaced. I'll add the test
There was a problem hiding this comment.
als I don't think that :icon:2 should be ignored should be ignored.
Please ensure your pull request adheres to the following guidelines:
This PR expands the ability to have numbers in icon names while keeping existing functionality. It does fail the regex vulnerability check at https://devina.io/redos-checker (but so does the existing). Will look at seeing if its possible to refactor to make it free of vulnerabilities by making this a tokenizer statemachine.
Related Issues
Thanks for contributing!