-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix555 verify path #557
Fix555 verify path #557
Conversation
…th doesn't exist; VerifyPath can also return a boolean now if it failed. joyfullservice#555
… a redundant sub, so I'm removing it. joyfullservice#555
The only thing left here is to determine if user should be prompted for a new path if |
@joyfullservice this is ready, btw. |
@joyfullservice let me know if you have any questions on this to merge. Thanks! |
@hecon5 - Just checking to see if you had seen my question above... I know sometimes these can get lost in the flow of notifications.... 😄 |
@joyfullservice I don't see any question on here, can you post it again? |
yup! I just looked through all the comments on this PR, the code comments (didn't see any here), and the related issue, but I cannot find any question about this. Can you link it or post it? |
End If | ||
|
||
Const FunctionName As String = ModuleName & ".FSO" | ||
Static RetryCount As Long |
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.
@hecon5 - Why would RetryCount
be declared as Static? The code below seems to indicate that the failure and retry would be within the scope of this function. If there is a reason we need to use Static, then shouldn't we reset the counter if there is no error?
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 long ago forgot why this was static, I seem to recall a one off bug (that may have been totally unrelated), but my notes on this have been lost to time.
Re-reading this it looks like it could be revised to Dim
instead of Static
. I'll do that now, as that will free up memory gremlins.
@hecon5 - Sorry, it might not have showed up at all for you till I "completed" the review. You should be able to see it now... (I think!) |
Aha! I see them now, looking at them now. |
@joyfullservice I updated the PR from your comment, let me know if you see anything else that needs looked at! |
@joyfullservice, could we get this merged? I actually ran into this bug that the PR fixes and it's quite ugly. It's not a "uh oh, an error happened" kind of error nor "oops, crashed. you lost the work. oh well!". It's more like "put on the hazmat and gas mask and clean out the bathroom from ceiling to floor and replace the toilet while you're at it" kind of error. |
Thank you @hecon5, and sorry for the delay on this! My family and I are in the midst of a cross-country move with the truck coming today, so that has kept things hopping around here... 😄 |
@joyfullservice, sorry about the inconvenient timing! I hope your move goes well! |
No worries! Things are going pretty well with the move. My wife's parents came for a week to help us pack, so that has been a huge blessing. I am grateful for your help in reviewing various changes, and your proposed suggestions have been helpful. |
Thanks @joyfullservice and @bclothier! |
This allows error handling for the
FSO
,VerifyPath
and a few others.