Directory creation error#4244
Directory creation error#4244jasonsanjose merged 10 commits intoadobe:masterfrom thefirstofthe300:directory-creation-error
Conversation
There was a problem hiding this comment.
Remove this if statement new if statement and just use suggestions above.
ghost
assigned
|
Initial review complete |
There was a problem hiding this comment.
At this else branch there is another error dialog that always display "File" in the strings as the code fixed. Maybe this could be fixed here too.
|
I implemented all code suggestions and everything should work like a charm. :) |
There was a problem hiding this comment.
You probably intended to remove DIRECTORY_ALREADY_EXISTS right?
|
@DaBungalow 3 more comments and we should be ready to merge. |
|
How does this look? |
There was a problem hiding this comment.
Shouldn't this be Strings.INVALID_FILENAME_TITLE instead of Strings.FILE_ALREADY_EXISTS_TILE (typo)?
There was a problem hiding this comment.
Oops. You are right.
There was a problem hiding this comment.
One other small Nit: errorString doesn't sound great as a variable name for this, specially since there is an errString variable. Maybe something like entryType or fileType would be better.
|
@DaBungalow 1 more comment. Please re-test. |
|
@jasonsanjose I would do the testing myself but I am currently running Linux only and am not sure how to get the test suite running on Linux. Can you point me in the right direction? Currently, all I see when I open SpecRunner.html is the title bar and nothing else. Edit: Looking at the SpecRunner.js, it would appear that I need to have Node installed, but I know for a fact that I don't. How to integrate? |
|
You could test manually to catch that last issue I caught. It's not ideal, but if you also have access to a mac or win machine that would help too. As you know, there's still plenty of work to do on linux. You can help by implementing node integration :) |
|
Not sure where to start to integrate Node. I don't have any experience On Wed, Jul 10, 2013 at 4:51 PM, Jason San Jose notifications@github.comwrote:
Danny Seymour |
|
OK. I implemented all of the comments and tested this. On my Linux build, this works perfectly, so it should be good to merge. |
There was a problem hiding this comment.
Almost. I went back to the original bug to test and found some more issues not mentioned.
If you create a file with a name foo, the try to create a directory of the same name, you get "The directory foo already exists" instead of "The file foo already exists"...and vice versa.
You can split the if here with an else if branch to handle TYPE_MISMATCH_ERR separate from PATH_EXISTS_ERROR, that should fix it.
|
One more comment @DaBungalow. Sorry I didn't catch that sooner. |
|
Is this what you are wanting? |
3 participants
I have two commits in this pull request because GitHub was being stupid and wouldn't let me make a pull request with my other branch.
This should fix issue #3985. I have checked it out on the Linux build so I am not a hundred percent sure but it appears to work just fine.