Fix #677 Error if project is in OneDrive #680
Conversation
| @@ -56,6 +56,9 @@ interface Parameters extends FlowParameters { | |||
| @Override | |||
| public void execute(Parameters parameters) { | |||
| Optional<Throwable> failure = parameters.getBuildResult().get().getFailure(); | |||
| if (System.getProperty("user.dir").contains("OneDrive")) { | |||
There was a problem hiding this comment.
We want to be checking the project directory, not the user directory. Because this would still error even if you move the project.
There was a problem hiding this comment.
Also, we'd want to do a case insensitive comparison.
There was a problem hiding this comment.
Both of these comments still need to be fixed.
| @@ -56,6 +59,10 @@ interface Parameters extends FlowParameters { | |||
| @Override | |||
| public void execute(Parameters parameters) { | |||
| Optional<Throwable> failure = parameters.getBuildResult().get().getFailure(); | |||
| if (Paths.get("").toAbsolutePath().toString().toUpperCase().contains("ONEDRIVE")) { | |||
There was a problem hiding this comment.
This is also not correct. There are gradle methods on Settings to get the location of the whole project. Those are what should be used.
| @@ -77,6 +78,9 @@ public void execute(Parameters parameters) { | |||
|
|
|||
| @Override | |||
| public void apply(Project project) { | |||
| if (project.getRootDir().toString().toUpperCase().contains("ONEDRIVE")) { | |||
There was a problem hiding this comment.
I'm still a bit worried about if this causes an issue when working in other languages. Wrap the contains portion in its own try catch that just catches and ignores any exception. But make sure the thrown OneDriveException isn't contained in that try catch.
There was a problem hiding this comment.
only place I could see it getting ignored is in checkBuildFailed. We might want to add a clause when checking for exceptions about this
There was a problem hiding this comment.
Apply is called at the very beginning. This is the right place to put it.
There was a problem hiding this comment.
If it’s not clear, I’m worried about toUpperCase throwing a weird exception in other languages. So if we try catch any exception potentially thrown by that whole statement, we cover our bases.
There was a problem hiding this comment.
I cant find anywhere else where the error would be ignored so should be good
There was a problem hiding this comment.
Ohh ok so it should be wrapped in a try catch
There was a problem hiding this comment.
Just the whole statement doing the comparison. You would need to throw the onedrive exception outside of that try catch so it’s not accidentally caught.
There was a problem hiding this comment.
No. That is not what I want. Just the contains statement should be in the try catch. The throw statement needs to still be outside the try catch. With a bool used to tell if it should be thrown or not.
There was a problem hiding this comment.
Right ok i'll fix that in my last commit
There was a problem hiding this comment.
Ok should be fixed now
| onedrive = true; | ||
| } | ||
| } catch(Exception e) { | ||
| System.out.println(e.toString()); |
There was a problem hiding this comment.
Don't print anything here. We just want to ignore any issue completely.
| @@ -77,6 +78,18 @@ public void execute(Parameters parameters) { | |||
|
|
|||
| @Override | |||
| public void apply(Project project) { | |||
| boolean onedrive = false; | |||
| try { | |||
| if (project.getRootDir().toString().toUpperCase().contains("ONEDRIVE")) { | |||
There was a problem hiding this comment.
You don't need the if here. You can just assign onedrive straight to the whole result of the contains statement.
| System.out.println(e.toString()); | ||
| } | ||
|
|
||
| if (onedrive == true) { |
There was a problem hiding this comment.
Just if (onedrive) {, don't need the == true.
| _project = project; | ||
| } | ||
|
|
||
| public void printError() { |
There was a problem hiding this comment.
Don't need the printError function, or to store the project variable.
Should Resolve #677