feat(core): Retrieve license from license file #53
Conversation
…e-auditor into license-from-license-file
…ge to license import
| const templates = { | ||
| [fs.readFileSync(`${__dirname}/templates/BSD-2-Clause.txt`).toString()]: | ||
| licenses.find((license) => license.licenseId === "BSD-2-Clause"), | ||
| [fs.readFileSync(`${__dirname}/templates/MIT.txt`).toString()]: licenses.find( | ||
| (license) => license.licenseId === "MIT", | ||
| ), | ||
| }; |
There was a problem hiding this comment.
please prove the code is actually functional by creating a sample directory with actual installed modules containing license files
you can find what @matt-jb used to query the npm registry for packages which do not contain licenses in package.json
we want to prove the code on actual data and actual use cases, not mocks which can prove to be unreliable
There was a problem hiding this comment.
Sure thing, here's the crawler's code: https://github.com/matt-jb/npm-registry-crawler
Just pop in some popular word to query packages for (like node, http, etc.) to get actual semi-random results.
The crawl is rather slow, though. It takes <20 mins for a full run of 1000 packages. This is due to the NPM's crawling policy, which rate-limits requests to 1 per second or less.
Here are some packages that I found yesterday that you can test against:
| const licenseFiles = [ | ||
| "LICENSE", | ||
| "LICENCE", | ||
| "LICENSE.md", | ||
| "LICENCE.md", | ||
| "LICENSE.txt", | ||
| "LICENSE-MIT", | ||
| "LICENSE.BSD", | ||
| ] as const; | ||
|
|
There was a problem hiding this comment.
this array was originally located in packages/core/src/license-finder/find-license-in-license-file.ts and that's where it should have stayed
I don't understand why you decided to ignore the separation of concerns that's been laid out
in 90% of the cases the license will be found in package.json
so I don't understand why this file has to know about the existence of various licenseFiles formats, when they most likely won't even be used
| for (const licenseFile of licenseFiles) { | ||
| const basicPath = path.join(packagePath, licenseFile); | ||
| const licenseFromLicenseFile = findLicenseInLicenseFile(basicPath); | ||
| if (licenseFromLicenseFile) { | ||
| return licenseFromLicenseFile; | ||
| } |
There was a problem hiding this comment.
same here, it was an architectural choice to offload the developer's cognitive load by having 3 simple code paths
this for loop should have been placed in findLicenseInLicenseFile - the file dedicated to retrieving licenses from license files
not here, where the specified strategies should only be called, not implemented
| if (!fs.existsSync(filename)) { | ||
| return; | ||
| } | ||
| const content = fs.readFileSync(filename).toString(); |
There was a problem hiding this comment.
let's make it async please because this code is executed in a loop
|
|
||
| export function findLicenseInLicenseFile( | ||
| filename: string, | ||
| ): LicenseWithPath | undefined { |
There was a problem hiding this comment.
let's make the return type just LicenseWithPath please
if it's not found return
license: undefined,
licensePath: undefined
| license: | ||
| Array.isArray(foundLicenses) && foundLicenses.length === 1 | ||
| ? foundLicenses[0] | ||
| : foundLicenses, |
There was a problem hiding this comment.
the needsVerification case which we discussed is not handled
also please move the condition out of the return statement for better readability
No description provided.