-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
TemporaryFolder.newFolder(String...) supports path separator #1406
TemporaryFolder.newFolder(String...) supports path separator #1406
Conversation
6763c81
to
288c6df
Compare
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.
Looks very good! I added a few suggestions inline.
directory = new File(directory, "temp3"); | ||
assertFileIsDirectory(directory); | ||
directory = new File(directory, "temp4"); | ||
assertFileIsDirectory(directory); |
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.
Wouldn't it be sufficient have a single assertion: assertFileIsDirectory(new File(tempFolder.getRoot(), "temp1/temp2/temp3/temp4")
?
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.
I wasn't 100% certain that would work on all platforms
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.
I'm pretty sure it should. Can we assume it does until proven otherwise?
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.
Sure. I have no way to verify on Windows though.
File file = root; | ||
boolean lastMkdirsCallSuccessful = true; | ||
for (int i = 0; i < paths.length; i++) { | ||
relativePath = relativePath == null ? new File(paths[i]) : new File(relativePath, paths[i]); |
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.
According to the Javadoc of File
this can be simplified to relativePath = new File(relativePath, paths[i])
.
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.
Thanks! Done.
if (new File(path).isAbsolute()) { | ||
throw new IOException("folder path \'" + path + "\' is not a relative path"); | ||
} | ||
} |
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.
I think we should also add a check for paths.length > 0
. Because in that case we currently return getRoot()
which isn't a "new fresh folder".
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.
Yes, I noticed that was the current behavior, too. I wasn't sure you wanted that in the same pull. I added the fix in a new commit. I also added a commit fixing some incorrect test method names.
288c6df
to
4a3cfd9
Compare
@@ -63,7 +63,7 @@ public void newFileWithGivenFilenameThrowsIllegalArgumentExceptionIfFileExists() | |||
} | |||
|
|||
@Test(expected = IllegalStateException.class) | |||
public void newFolderThrowsIllegalStateExceptionIfCreateWasNotInvoked() | |||
public void newFolderThrowsIOExceptionIfCreateWasNotInvoked() |
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.
This one actually does throw an IllegalStateException
.
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.
Woops! Fixed
4a3cfd9
to
109397b
Compare
The error messages on failures now include the full relative path that could not be created.
109397b
to
70a86fb
Compare
/* | ||
* Before checking if the paths are absolute paths, check if create() was ever called, | ||
* and if it wasn't, throw IllegalStateException. | ||
*/ |
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.
I think the comment is wrong because I could not find the IllegalStateException
.
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.
getRoot()
throws IllegalStateException
if before()
wasn't (yet?) called.
That's a nice improvement. Thanks. |
No description provided.