-
-
Notifications
You must be signed in to change notification settings - Fork 336
Feat workspace filter #1455
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: main
Are you sure you want to change the base?
Feat workspace filter #1455
Conversation
webpro
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.
Thanks for the pull request! This is a valuable feature for large projects indeed. Happy to review, but given the size of the changeset and the fact I'm not sure we should include the Git-based range filter (at least not yet), could you please remove that part?
| if (this.workspace) { | ||
| const dir = path.resolve(this.cwd, this.workspace); | ||
| if (!isDirectory(dir)) throw new ConfigurationError('Workspace is not a directory'); | ||
| if (!isFile(dir, 'package.json')) throw new ConfigurationError('Unable to find package.json in workspace'); |
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.
Are we giving up on this? If someone does knip -W ./not-found or if there's no package.json then Knip should err.
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.
Yes, an error will be reported when parsing the path.
| const isSelectedFilePath = createWorkspaceFilePathFilter(this.cwd, selectedWorkspaces, availableWorkspaceNames); | ||
| if (!isSelectedFilePath) return; | ||
| this.workspaceFilter = (filePath: string) => !isSelectedFilePath(filePath); | ||
| } |
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.
How about creating the filter earlier on, e.g. in createOptions? Then we can set in the constructor directly. Perhaps same for as much as possible for the logic happening in ConfigurationChief.
Regarding Git range filtering, my consideration is that it can conveniently check and report the scope of changes in the CI environment of monorepo projects, thus shortening the execution time. |
Feature #1441
Filter the workspace by package names, directory paths, globs and Git ranges.
Key Changes
--workspace(-W) arguments.!.[main...HEAD]).