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.

Sign up for GitHub

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

Jump to bottom

cleanups: replaced File#delete with Files#delete #11762

Merged
merged 1 commit into from
Oct 25, 2023
Merged

cleanups: replaced File#delete with Files#delete #11762

merged 1 commit into from
Oct 25, 2023

Conversation

LostArtist
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run

  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.

  • Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.

  • ⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@essobedo
Copy link
Contributor

Are there any reasons why we should do that?
According to the Javadoc, the main interest in using Files#delete is for error reporting, to diagnose why a file cannot be deleted but in this PR you mostly don't care about the exception thrown so it is not really useful. Moreover, it causes some known behavior changes and finally dealing with exceptions affects the performances so I tend to vote -1 for this change unless you have good reasons

orpiske
orpiske requested changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@orpiske orpiske left a comment

Choose a reason for hiding this comment

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

I'll review more carefully tomorrow, but I think for now it would be OK to have it only on the TestSupport (w/ proper error handling) as both the performance impact would be smaller and the potential benefit greater.

As for others, this should probably come along with code that adjusts the calls to actually read the failure/errors from Files#delete.

In short: adjust the code for the TestSupport and leave the others for a subsequent PR with the correct error handling.

@github-actions github-actions bot removed the dsl label Oct 19, 2023
@orpiske
Copy link
Contributor

orpiske commented Oct 25, 2023 •
edited by github-actions bot
Loading

/component-test camel-file

Result ✅ The tests passed successfully

@github-actions
Copy link
Contributor

🤖 The Apache Camel test robot will run the tests for you 👍

@orpiske
Copy link
Contributor

orpiske commented Oct 25, 2023 •
edited by github-actions bot
Loading

/component-test camel-kafka

Result ✅ The tests passed successfully

@github-actions
Copy link
Contributor

🤖 The Apache Camel test robot will run the tests for you 👍

@orpiske orpiske merged commit b7831e3 into apache:main Oct 25, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orpiske orpiske orpiske approved these changes

Assignees
No one assigned
Labels
camel-4 components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LostArtist @essobedo @orpiske