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

Remove delete_recursively #190

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Conversation

shonfeder
Copy link
Contributor

Looks like this was copied from Lwt at some point. It is not used within obuilder currently, nor have I found it used in any of our other projects. And it has since been exposed in Lwt. See https://github.com/ocsigen/lwt/blob/48abed72467ca7479e95f1be06d02f40c7c434bd/CHANGES#L25

Copy link

@punchagan punchagan left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for taking care of this.

From the placement of the comment, it looks like the win32_unlink and unlink may also be artifacts of the delete_recursively function. I took a quick look and didn't find uses of this unlink function too. If it's not being used anywhere, could those go too?

@MisterDA
Copy link
Contributor

I confirm that all these functions can be removed now that they've been exposed from Lwt.

Looks like this was copied from Lwt at some point. It is not used within
obuilder currently, nor have I found it used in any of our other
projects. And it has since been exposed in Lwt. See
https://github.com/ocsigen/lwt/blob/48abed72467ca7479e95f1be06d02f40c7c434bd/CHANGES#L25
@shonfeder
Copy link
Contributor Author

Thank you for the reviews (and for spotting the extra dead code, @punchagan)!

@shonfeder
Copy link
Contributor Author

The CI errors are spurious, and recorded in issues.

@shonfeder shonfeder merged commit fc345f5 into ocurrent:master Sep 19, 2024
6 of 9 checks passed
@shonfeder shonfeder deleted the remove-dead-code branch September 19, 2024 20:40
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.

3 participants