refactor: split search and fitting to status functionality #127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # packages/core/src/audit-licenses.ts
# Conflicts: # README.md # package-lock.json # packages/cli/src/components/audit-licenses/audit-licenses.tsx # packages/core/src/audit-licenses.ts # test/test/npm.test.ts # test/testProjects/monorepo/package-lock.json # test/testProjects/monorepo/package.json
Lukasz-pluszczewski
changed the title
Lukasz-pluszczewski
changed the title
F-Kublin
approved these changes
Dec 9, 2024
Comment on lines
+83
to
+90
| if (!packageJsonResult.success) { | ||
| //To code reviewer: Should we handle in in this or different way? | ||
| errorResults.set(packageNameFromPath, { | ||
| packagePath, | ||
| errorMessage: packageJsonResult.errorMessage, | ||
| }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Current solution looks ok, what other way do you have in mind?
There was a problem hiding this comment.
I was curious here, if we should add this error to the new errorResults map here.
Comment on lines
+55
to
+67
| // it("should return the correct license when file name matches a license name", () => { | ||
| // const filePath = "LICENSE-Apache-2.0"; | ||
| // const expectedLicense = LicenseSchema.parse(licenseMap.get("Apache-2.0")); | ||
| // const result = retrieveLicenseFromLicenseFileName(filePath); | ||
| // expect(result.licenses).toEqual([expectedLicense]); | ||
| // }); | ||
|
|
||
| it("should return an empty array when file name does not match any known license pattern", () => { | ||
| const filePath = "LICENSE-UNKNOWN"; | ||
| const result = retrieveLicenseFromLicenseFileName(filePath); | ||
| expect(result.licenses).toEqual([]); | ||
| }); | ||
| }); | ||
| // it("should return an empty array when file name does not match any known license pattern", () => { | ||
| // const filePath = "LICENSE-UNKNOWN"; | ||
| // const result = retrieveLicenseFromLicenseFileName(filePath); | ||
| // expect(result.licenses).toEqual([]); | ||
| // }); | ||
| // }); |
There was a problem hiding this comment.
What's the situation with these tests? Should they be removed, uncommented, refactored?
There was a problem hiding this comment.
I think we should remove the tests related to file names, as this functionality is removed in this PR.
pawelkoniecznybh
commented
Dec 9, 2024
| @@ -66,6 +66,7 @@ export default function AuditResult({ | |||
| warning, | |||
| overrides, | |||
| }: AuditResultProps) { | |||
| //TODO handle errorResults here | |||
There was a problem hiding this comment.
I think we should implement this in this PR.
| overrides: overrides, | ||
| warning: warning, | ||
| needsUserVerification: needsUserVerification, | ||
| errorResults: errorResults, |
There was a problem hiding this comment.
It can be written like this:
return {
groupedByStatus,
notFound,
overrides,
warning,
needsUserVerification,
errorResults,
};
|
|
||
| if (!packageJsonResult.success) { | ||
| //To code reviewer: Should we handle in in this or different way? | ||
| errorResults.set(packageNameFromPath, { |
There was a problem hiding this comment.
Should we also set process.exitCode = 1; in this case here?
bh-pwalen
approved these changes
Dec 9, 2024
Lukasz-pluszczewski
force-pushed
the
feat-split-functionality
branch
from
15a69a4 to
0618e74
Compare
Lukasz-pluszczewski
force-pushed
the
feat-split-functionality
branch
from
9234229 to
c627f78
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.