Skip to content

Prohibit use of Files util class from AssertJ to create temporary directories#24931

Merged
wendigo merged 4 commits intotrinodb:masterfrom
ksobolew:kudi/temp-dir
Feb 10, 2025
Merged

Prohibit use of Files util class from AssertJ to create temporary directories#24931
wendigo merged 4 commits intotrinodb:masterfrom
ksobolew:kudi/temp-dir

Conversation

@ksobolew
Copy link
Copy Markdown
Contributor

@ksobolew ksobolew commented Feb 6, 2025

Description

@TempDir should be used where possible, and Files from the JDK otherwise.

Additional context and related issues

There are plenty of other places where @TempDir could be used - left as a follow-up.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

These changes are all related, so they are in a separate commit not to
mix with others.
This is a general modernization, as from replacing the `Files` class
with `@TempDir` follows replacing `File` with `Path` and related changes
like using `Files#copy` from the JDK instead of from Guava, and from
most of that follows better (more explicit) exception handling. And all
of this is tightly intertwined.
@cla-bot cla-bot bot added the cla-signed label Feb 6, 2025
@github-actions github-actions bot added the iceberg Iceberg connector label Feb 6, 2025
@ksobolew ksobolew requested review from kokosing and wendigo February 6, 2025 12:33
...to create temporary directories, at least. `@TempDir` should be used
where possible, and `Files` from the JDK otherwise.
@ebyhr ebyhr requested a review from martint February 6, 2025 23:19
@martint
Copy link
Copy Markdown
Member

martint commented Feb 6, 2025

Move the removals of throws Exception to a separate commit. They are generally unrelated to the TempDir change and add make it hard to see the essence of the changes.

@ksobolew
Copy link
Copy Markdown
Contributor Author

ksobolew commented Feb 7, 2025

Move the removals of throws Exception to a separate commit. They are generally unrelated to the TempDir change and add make it hard to see the essence of the changes.

They are related in a sense that the JDK Files class tends to throw IOException where other alternatives do not. But I'll see what I can do.

@martint
Copy link
Copy Markdown
Member

martint commented Feb 7, 2025

If they are related, it’s ok to leave them. I thought I saw some places where they were being removed, but I may have misread the change.

Copy link
Copy Markdown
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

LGTM

@wendigo wendigo merged commit 0d43cb9 into trinodb:master Feb 10, 2025
@github-actions github-actions bot added this to the 471 milestone Feb 10, 2025
@ksobolew ksobolew deleted the kudi/temp-dir branch February 10, 2025 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed iceberg Iceberg connector

Development

Successfully merging this pull request may close these issues.

4 participants