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

[FileSystem] Simplify and modernize NativeHandlers #1441

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

No description provided.

Copy link
Contributor

Test Results

 1 734 files  ±0   1 734 suites  ±0   1h 26m 47s ⏱️ + 2m 2s
 3 971 tests ±0   3 949 ✅ ±0   22 💤 ±0  0 ❌ ±0 
12 510 runs  ±0  12 349 ✅ ±0  161 💤 ±0  0 ❌ ±0 

Results for commit c868e15. ± Comparison against base commit 6f2754b.

Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

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

The title "modernize" is to vague. Please explicitly describe what you have done - and why. i don't even get why Path.of() should be any better.
The implicit exists() check seems to be good!

@@ -74,6 +74,8 @@ public void assumeSymbolicLinksAvailable() throws Exception {
FileSystemHelper.canCreateSymLinks());
}

// TODO: test hard-links too?
Copy link
Contributor

Choose a reason for hiding this comment

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

remove todo


try {
BasicFileAttributes readAttributes = Files.readAttributes(path, BasicFileAttributes.class);
info.setExists(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a junit test if this behaves like before for hidden files as the code contains comments that opening hidden files can lead to FileNotFoundException. See uses of org.eclipse.core.filesystem.EFS.ATTRIBUTE_HIDDEN

Copy link
Contributor

Choose a reason for hiding this comment

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

you can look at https://github.com/eclipse-platform/eclipse.platform/pull/1444/files#diff-81300c5bdd85b0fca987936ae34590fbf4bbf835119e1efd68698461d230b817R252 how to create a hidden file.
But to me it is not clear how/if the Defaulthandler is tested in CI

@HannesWell
Copy link
Member Author

I'll have a look at the tests.

i don't even get why Path.of() should be any better.

Paths.get() was only introduced because back then static methods on interfaces were not supported. When it was possible Path.of was added, as a method that is easier to discover/remember/read. And since both are technical equal I don't see a reason to not use it.

@jukzi
Copy link
Contributor

jukzi commented Jun 25, 2024

And since both are technical equal I don't see a reason to not use it.

I would also use Path for new code, but is it worth to enlarge the git history with rewriting it?

@akurtakov
Copy link
Member

I would also use Path for new code, but is it worth to enlarge the git history with rewriting it?

As the PR is not just about that and what is questioned removes one useless method call from the chain - it is worth it.

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