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

IO-552: Honor tilde as a valid character for file and directory names #297

Closed
wants to merge 3 commits into from

Conversation

wodencafe
Copy link
Contributor

This pull request is meant to address https://issues.apache.org/jira/projects/IO/issues/IO-552 by treating tilde ~ as a valid character for file and directory names.
https://issues.apache.org/jira/projects/IO/issues/IO-552

@garydgregory
Copy link
Member

Thank you for your PR @wodencafe.

@kinow: Any thoughts on behavior compatibility here that is not captured in tests? I am thinking especially about the different treatment of ~ on Linux and Windows.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

I think it fixes the issue without breaking other methods that use the getPrefixLength. One comment to see if we could simplify the change. But I couldn't find any issue with the implementation or tests @wodencafe , @garydgregory .

Thanks @wodencafe !

src/main/java/org/apache/commons/io/FilenameUtils.java Outdated Show resolved Hide resolved
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

@wodencafe I believe your change to concat wouldn't impact the test cases you modified, but it doesn't hurt having the extra checks.

From what I could see, your tests all look correct to me, and the change to concat appears to be in order as well.

So +1, and thank you for your contribution and for the patience with the review process 👍

Bruno

@wodencafe
Copy link
Contributor Author

@kinow Do you have any idea why the macos-latest, Java 11 CI would fail? The test that failed does not appear to be related to the changes made in this PR, from what I can tell 🤔

@kinow
Copy link
Member

kinow commented Nov 11, 2021

@kinow Do you have any idea why the macos-latest, Java 11 CI would fail? The test that failed does not appear to be related to the changes made in this PR, from what I can tell thinking

No idea @wodencafe , sorry. I started looking at Commons IO recently (normally focus on other components, but am on a sabbatical, so more spare time), and haven't seen the error yet.

But agree that it doesn't appear to be related to your change. For others, the failure:

Error:  Tests run: 3, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.208 s <<< FAILURE! - in org.apache.commons.io.FileUtilsWaitForTest
Error:  org.apache.commons.io.FileUtilsWaitForTest.testWaitForInterrupted  Time elapsed: 0.02 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:35)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:179)
	at org.apache.commons.io.FileUtilsWaitForTest.testWaitForInterrupted(FileUtilsWaitForTest.java:56)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:149)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:140)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:84)
	at org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.execute(JUnitPlatformProvider.java:188)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invokeAllTests(JUnitPlatformProvider.java:154)
	at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:124)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:428)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:562)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:548)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 87.791% when pulling 4764981 on wodencafe:IO-552-Concat into d0233bf on apache:master.

@garydgregory
Copy link
Member

I'm still not sure about this one since ~ is a shell convention to expand the home directory as opposed to anything to do with the actual file system. Needs further thought...

@garydgregory
Copy link
Member

I am still -1 on this one because ~ is only a Unix-like shells and ~.txt is a legal file name on Windows 10. So I will close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants