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

feat: builtin funtion rm. #401

Closed
wants to merge 4 commits into from

Conversation

MuhamedMagdi
Copy link
Contributor

It uses the -rf flags by default.

ref #215

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Aug 11, 2024

I'm thinking... can we insert some kind of runtime detection for removing some directories like /, ~, /bin, /sbin, /dev, /etc, /usr etc.

To prevent this command from running - this is usually not the intended use of this command. If user really wants to remove the /, then they must use the command $rm -rf /$ syntax.

@Mte90
Copy link
Member

Mte90 commented Aug 12, 2024

maybe a prefilled parameter that the developer can define, the code automatically should check if the path is part of that and shows an alert.

@Ph0enixKM
Copy link
Member

@b1ek @KrosFire What do you think?

@KrosFire
Copy link
Member

Bash scripts are pretty unsafe by design. I would allow this as it is, it's just a syntax sugar for what you can already do.

But I'm thinking, if we could create some kind of "secure compile" mode or "debug", where we create script but with something like strace so that someone can be sure, that the script is not doing anything funky. But that's a topic for another day

@Mte90
Copy link
Member

Mte90 commented Aug 18, 2024

@KrosFire there is #219 for debug more :-)

@RokkuCode
Copy link

RokkuCode commented Aug 18, 2024

Having a built in "rm" command that has "-rf" by default is for me a violation of principle of last suprise. default rm does not work like this. why should the build in rm more dangerous than the $rm -frv /$? The developer who wrote this command stmt wants explicitly use this dangerous command, but the user who uses the built-in rm command does not. the builtin rm keyword should work as the rm on the shell, if not dont call it rm. name it deleteFile/deletePath and provide it via the standard library.

in the end i dont see any benefit of having it as built in. it is better placed as part of the std library.

@Mte90
Copy link
Member

Mte90 commented Aug 19, 2024

I think that needs just one parameter, to remove folders and a sanitization for the path.
I think that other languages just do that, it is a task for the developer to do more.
We can extend the rm builtin in the stdlib with a safety version later that does more checks.

@Ph0enixKM
Copy link
Member

Okay. My stance on this is that if this builtin is destructive (dangerous) then let's keep it as a function library.

I'm closing it and will update the built-in guidelines. If you think otherwise - please reopen and we can have a discussion on it.

@Ph0enixKM Ph0enixKM closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants