-
-
Notifications
You must be signed in to change notification settings - Fork 18
Tighten vimrc regex #41
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughRefined the vimrc regex in Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lambdalisue
left a comment
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.
The solution here is just to slap a negative search for paths at it rather than using a .*. I did include backslashes in the group, but noticed later that none of the other paths include backslashes. If the paths are indeed normalised to forward slashes, let me know and I'll drop those from the regex
I’d forgotten, but it doesn’t make sense to support Windows paths only for .vimrc. Let’s remove them for now and open a separate issue for Windows support if needed.
assets/json/pattern.json
Outdated
| ".*mootools.*\\.js$": "", | ||
| ".*require.*\\.js$": "", | ||
| ".*vimrc.*": "", | ||
| ".*vimrc[^/\\\\]*$": "", |
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.
It looks like this pattern would also match HELLOvimrc// or WORLDvimrc\\\\\. Is accepting trailing slashes or backslashes intentional? I suspect you might have meant something like: .*vimrc\\(/.*\\)\\?$
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.
It wouldn't. [^] denotes a negative character capture group, i.e. matching everything except those characters. See https://regex101.com/r/7htfk4/2
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.
Oops. Sorry, I might drunk. But then, it hits with hellovimrcworld as well? Is that what you expect? I feel it doesn't make sense. For example, .*vimrc\..* make a little bit sense to me (well, actually, leading .* doesn't make sense to me though. I (or someone) might mean \.vimrc\..* but I don't remember the context)
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.
Leading stuff does make some sense. .lvimrc (from a plugin) and .gvimrc should be covered for example. The proper tightening of it matching those forms would probably be (.*/)?[._].vimrc$, but that probably has collateral damage if someone uses the postfix form for whatever reason
Per PR review
7dee990 to
3031d8c
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/nerdfont/path/pattern.vimspec (1)
19-20: Add Windows-style path coverageThe regex now guards against both
/and\, so we should also assert that a Windows-stylevimrc-modules\Vagrantfilestill resolves to the Vagrant glyph. That safeguards us if upstream ever stops normalizing to forward slashes.let glyph = nerdfont#path#pattern#find('vimrc-modules/Vagrantfile') Assert Equals(glyph, '') + let glyph = nerdfont#path#pattern#find('vimrc-modules\Vagrantfile') + Assert Equals(glyph, '')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assets/json/pattern.json(1 hunks)test/nerdfont/path/pattern.vimspec(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/json/pattern.json
Closes #40 - I wish I had seen this issue three months ago, but managed to miss it (thanks for notifying me about everything except the things I'm interested in, GitHub), so here I am three months late
The solution here is just to slap a negative search for paths at it rather than using a
.*.I did include backslashes in the group, but noticed later that none of the other paths include backslashes. If the paths are indeed normalised to forward slashes, let me know and I'll drop those from the regexDropped per reviewSummary by CodeRabbit
Bug Fixes
Tests