-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat(no-redundant-files): add new rule #721
base: main
Are you sure you want to change the base?
feat(no-redundant-files): add new rule #721
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
==========================================
+ Coverage 97.62% 98.00% +0.38%
==========================================
Files 17 18 +1
Lines 1177 1404 +227
Branches 112 132 +20
==========================================
+ Hits 1149 1376 +227
Misses 28 28 ☔ View full report in Codecov by Sentry. |
fd602d4
to
98de911
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.
Whoohoo, this is awesome! 🥳 Very excited to see this working.
I ended up leaving comments mostly around refactoring for readability. Please push back if you think I'm being too nitpicky. But either way the functionality looks very good to me!
Thanks!
Uh oh! @michaelfaith, at least one image you shared is missing helpful alt text. Check https://github.com//pull/721#issuecomment-2594302487 to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
ebb190a
to
018d6b1
Compare
Aside: interesting bug in the action 🤔. Filed github/accessibility-alt-text-bot#62. |
Ha, yeah, it does feel odd. I do have a preference for the perfectionist plugin workin on the other parts of rule files though. Do you see a good way to configure it to always put In the meantime I'd suggest putting an inline ESLint disable comment on the line. |
const cachedRegex = new Map<string, RegExp>(); | ||
const getCachedLocalFileRegex = (filename: string) => { | ||
// Strip the leading `./`, if there is one, since we'll be incorporating | ||
// it into the regex. | ||
const baseFilename = filename.replace("./", ""); | ||
let regex = cachedRegex.get(baseFilename); | ||
if (regex) { | ||
return regex; | ||
} else { | ||
regex = new RegExp(`^(./)?${baseFilename}$`, "i"); | ||
cachedRegex.set(baseFilename, regex); | ||
return regex; | ||
} | ||
}; |
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.
[Refactor] I've seen this "make a Map for a single cache" pattern a bunch, I made a cached-factory
package for it. Up for using it to reduce code here? IMO this is a bit more readable:
const cachedRegex = new Map<string, RegExp>(); | |
const getCachedLocalFileRegex = (filename: string) => { | |
// Strip the leading `./`, if there is one, since we'll be incorporating | |
// it into the regex. | |
const baseFilename = filename.replace("./", ""); | |
let regex = cachedRegex.get(baseFilename); | |
if (regex) { | |
return regex; | |
} else { | |
regex = new RegExp(`^(./)?${baseFilename}$`, "i"); | |
cachedRegex.set(baseFilename, regex); | |
return regex; | |
} | |
}; | |
import { CachedFactory } from "cached-factory"; | |
const regexFactory = new CachedFactory( | |
(baseFilename: string) => new RegExp(`^(./)?${baseFilename}$`, "i"), | |
); | |
const getLocalFileRegex = (filename: string) => | |
regexFactory.get(filename.replace("./", "")); |
(not a blocker at all)
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.
Right on. Do you think it's worth the additional dependency?
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.
I'm a little biased because it's my package 😄 but yeah I do. It's very small.
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.
This is shaping up very nicely! There's only one blocking request from me, the [Bug] from filtering away invalid elements in arrays.
#721 (comment) is a fun discussion and I'm enjoying iterating on the design. But as soon as you don't have energy/time to continue it, no worries, it's not blocking.
I put in a change to allow |
e8de066
to
ed9f959
Compare
This change adds a new rule for preventing inclusion of unnecessary or redundant files in a package.json's `files` list. It checks for two primary types of errors: 1. Duplicate entries within the files array (the same file listed more than once) 1. Files that are automatically included by npm, and don't need to be explicitly included. Of the second type, there are two flavors that npm includes automatically 1. Files that are always included, regardless of what else is present in the package.json (e.g. README.md) 1. Files that are included because they're declared in other places in the package (e.g. file declared as the `main` entry)
ed9f959
to
bd91522
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.
🚀 Awesome! This is looking great, and I'm really excited to get it in. Thanks so much for iterating on it a bunch @michaelfaith, you did great work here!
I can go ahead and merge this whenever you're happy with it. Since we have an open thread I'll wait for your decision on what to do about it. If you think it's perfect as-is, great, I can take it from here. 👍
PR Checklist
status: accepting prs
Overview
This change adds a new rule for preventing inclusion of unnecessary or redundant files in a package.json's
files
list. It checks for two primary types of errors:Of the second type, there are two flavors that npm includes automatically
main
entry)The one thing I wasn't sure about, is whether this new rule should be included in recommended or not (which would technically be a breaking change...)
Closes: #686