-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
CLI: Fix "reset" script removing .idea directory and add "cleanup" script #18309
Conversation
It also makes sure only root level directories are excluded Closes #18308
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 8bdb1cd. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
@@ -11,8 +11,8 @@ const cleaningProcess = spawn('git', [ | |||
'clean', | |||
'-xdf', | |||
'-n', | |||
'--exclude=".vscode"', | |||
'--exclude=".idea"', | |||
'--exclude="/.vscode"', |
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.
@okonet Any idea why this is necessary? This seems to be more restrictive than what was there before
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.
Without that it would try to match .vscode
recursively which would increase the amount of directories it would output dramatically because .vscode
is often present in node_modules
. With the /
it would only match the root directories.
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 don't even think /.vscode
is needed here since it's under VCS but I left it in case it contains some user-generated settings.
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.
Fair enough, but I was more interested in '--exclude=".idea"',
which already existed in the code and was replaced by --exclude="/.idea"
which seems more restrictive
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 .idea
didn't work for me at all. Thus the fix.
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.
That's my question. Any idea why ".idea"
doesn't work but "/.idea"
does?
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.
Not really. I would assume it could treat the . as part of regex but I didn't want to spend too much time investigating.
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.
LGTM
Issue: #18308
What I did
Fix the script to not delete
.idea
Fix the wording in case when the user decides not to run the script
Add an additional script to delete
**/dist
onlyHow to test
Create
.idea
dir in the rootRun
yarn bootstrap
and selectreset
After script has run you should still see
.idea
in the root directoryRun
yarn bootstrap
and selectreset
scriptAnswer
No
or press Esc when asked "are you sure?"It should say
Cleanup cancelled
instead ofproblem is between keyboard and chair
Run
yarn bootstrap
and selectcleanup
scriptIt should only delete
/dist
folders inside packages and leavenode_modules
and other files intact