Skip to content
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

Add support for moving folders to system trash #113

Closed
wants to merge 3 commits into from

Conversation

Fleker
Copy link

@Fleker Fleker commented Oct 16, 2021

Adds a --trash flag to the CLI to move a node_modules to system trash rather than deleting directly.

To use the trash module, the project needs to be rewritten to use ESModules
There is one test still outstanding that isn't running as I work out some compat issues, but wanted to create this now to solicit some feedback.

See #60

The 'trash' NPM package is added
This dependency uses ESModule, requiring project refactoring
Fixes voidcosmos#60
@zaldih zaldih self-requested a review October 30, 2021 08:20
@zaldih zaldih added the enhancement New feature or request label Oct 30, 2021
@zaldih zaldih self-assigned this Oct 30, 2021
Copy link

@0ex-d 0ex-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one adding the --trash option but if adding one single feature could make rewriting most of the original project structure, i don't think it is ideal for a merge. You can look for a typescript port for trash lib or an alternative package.

Comment on lines +1 to +7
export * from './cli.constants.js';
export * from './main.constants.js';
export * from './messages.constants.js';
export * from './sort.result.js';
export * from './spinner.constants.js';
export * from './update.constants.js';
export * from './recursive-rmdir-node-support.constants.js';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious why suffix file names with .js extension?

Comment on lines +1 to +9
export * from './console.service.js';
export * from './files.service.js';
export * from './https.service.js';
export * from './linux-files.service.js';
export * from './results.service.js';
export * from './spinner.service.js';
export * from './stream.service.js';
export * from './update.service.js';
export * from './windows-files.service.js';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh?

@zaldih
Copy link
Member

zaldih commented Oct 9, 2022

Hi!
It's been a while since I had to decide what to do whith this precious PR. The problem is that initially I didn't really understand why it was necessary to modify so many things just for the feature, so one of the tasks I thought to do was to "upate" the other lib to make it compatible with projects like this. Actually the one who was wrong was me since the future points to ESM although eve today it is still experimental.
That is why I finally decided to update the project with ESM (already in develop branch).

Now that there should be no more ESM problems, could you reopen the PR with only the changes related to the feature? I will merge it as soon as possible. Thank!

@zaldih zaldih closed this Oct 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants