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

Fix exists? method to find task in given path and if path or folder not present return false #40

Conversation

Vasu1105
Copy link

@Vasu1105 Vasu1105 commented Jun 6, 2018

Signed-off-by: vasu1105 vasundhara.jagdale@msystechnologies.com

… folder not present return false

Signed-off-by: vasu1105 <vasundhara.jagdale@msystechnologies.com>
@Vasu1105 Vasu1105 requested a review from btm June 6, 2018 09:29
@@ -193,7 +193,30 @@ def enum
# Returns whether or not the specified task exists.
#
def exists?(task)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably change task here to something like full_task_path so you aren't using task for two different things in this method (the fully qualified task path at first, and then an object returned by root.GetTask() later.)

task_name = nil

if task.include?("\\")
*path, task_name = task.split("\\")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment here like "# Use the splat operator put all folder elements into path and leave only the task name`

return false
end

task = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

if you can avoid using task for two different purposes in this method you can drop this nil.

rescue WIN32OLERuntimeError => err
return false
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

what gets returned by this method here if root is nil? We seem to be returning true from line 215, task && task.Name == task_name but if for some reason root is false this block isn't run and we don't explicitly return anything. If we add an explicit return false to the end, we need to also add a return true after line 215.

Copy link
Contributor

@btm btm left a comment

Choose a reason for hiding this comment

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

a couple small comments

Signed-off-by: vasu1105 <vasundhara.jagdale@msystechnologies.com>
@btm btm merged commit 46346e2 into chef:ole Jun 7, 2018
Nimesh-Msys pushed a commit to MsysTechnologiesllc/win32-taskscheduler that referenced this pull request Jun 27, 2018
…ot present return false (chef#40)

* MSYS-834 Fix exists? method to find task in given path and if path or folder not present return false

Signed-off-by: vasu1105 <vasundhara.jagdale@msystechnologies.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants