Skip to content

Add error message to TemporaryFolder.newFolder("String/String") #959

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

Closed
wants to merge 1 commit into from

Conversation

cfontes
Copy link

@cfontes cfontes commented Jul 22, 2014

Adding error message explaining that you can't create a tempFolder using "Parent/Folder" as one string.

@@ -1,18 +1,15 @@
<?xml version="1.0" encoding="UTF-8"?>
<projectDescription>
<name>junit</name>
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the changes to this file:

git checkout master .project

Ditto for the other non-java files

@cfontes
Copy link
Author

cfontes commented Jul 22, 2014

Ops, Sure thing...

Thanks for the review !

*/
private void validateIOSeparator(String folderName) throws IOException {
if (folderName.contains(File.separator)) {
throw new IOException(String.format("%s%s%s%n%s",
Copy link
Member

Choose a reason for hiding this comment

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

It's not very readable to use %s to format constant strings. I was suggesting that you could use String.format so you could use File.separator in the message without concatenating strings.

@kcooney
Copy link
Member

kcooney commented Jul 22, 2014

Thanks for the changes!

A few more minor changes. After committing them, could you squash your commits? See https://github.com/junit-team/junit/wiki/Pull-Requests for instructions

thrown.expect(IOException.class);
thrown.expectMessage(String.format("%s%s%s%n%s",
"It's not possible to use the OS separator to create folder hierarchies like 'MyParentFolder'",
File.separator, "'MyFolder'.", "Please use newFolder('MyParentFolder', 'MyFolder') instead"));
Copy link
Member

Choose a reason for hiding this comment

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

Also, let's not put a line break in the message. Thanks.

@marcphilipp
Copy link
Member

Validating for File.separator is fine. However, Java on Windows also understands / as separator. Should we forbid that as well?

@cfontes
Copy link
Author

cfontes commented Jul 29, 2014

If it accepts it. I think we should.

Any other special cases?

@kcooney
Copy link
Member

kcooney commented Jul 30, 2014

One way to see if a path segment is valid is to use it to create a File and see if get File.parent() returns non-null. The error message could say something like "not a path segment' and then include the suggested workaround

@cfontes
Copy link
Author

cfontes commented Aug 6, 2014

Working on improving that;

@kcooney
Copy link
Member

kcooney commented Aug 13, 2014

Fixed with #974

@kcooney kcooney closed this Aug 13, 2014
@cfontes
Copy link
Author

cfontes commented Aug 13, 2014

Hey, guys.

What happened?

@stefanbirkner
Copy link
Contributor

Please read @kcooney's comment.

@cfontes
Copy link
Author

cfontes commented Aug 13, 2014

I've read it.

I just don't understand what happened... I am kind of new. Thanks!

@stefanbirkner
Copy link
Contributor

@priav03 created a different pull request (#974) that fixes the problem. This pull request has been merged. Hence @kcooney closed your pull request because it is not needed anymore.

Nevertheless thanks for your work on this issue.

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

Successfully merging this pull request may close these issues.

4 participants