-
Notifications
You must be signed in to change notification settings - Fork 114
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
base: master
Are you sure you want to change the base?
[FileSystem] Simplify and modernize NativeHandlers #1441
Conversation
There was a problem hiding this 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? |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
I'll have a look at the tests.
|
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. |
No description provided.