-
Notifications
You must be signed in to change notification settings - Fork 8
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 144 Add cop warning for sleep #164
Conversation
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.
Looks good. A couple of suggestion to description and additional tests.
module RuboCop | ||
module Cop | ||
module SketchupSuggestions | ||
# Avoid kernel `sleep` as it freezes up SketchUp. |
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.
How about:
Avoid kernel
sleep
as it makes SketchUp unresponsive for the duration of the sleep. This is a poor user experience as it's difficult for the end user to determine if the application stopped working or not.
PreferUI.start_timer
or a callback from the resource you are waiting for.
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 think "freezes SketchUp" is a more concise way to put the first part.
It's the "callback for the resource you are waiting for" part I'm unsure about. Is "callback for" better than "callback for"?
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.
Thoughts on this suggestion?
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.
Is "callback for" better than "callback for"
looks the same to me... ?
Test |
# for. | ||
class Sleep < SketchUp::Cop | ||
MSG = '`sleep` freezes up SketchUp. Prefer `UI.start_timer` or a '\ | ||
'callback for the resource you are waiting for.' |
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.
Not sure how to best phrase this sentence. A callback for or a callback from?
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.
"for" sounds good to me
module RuboCop | ||
module Cop | ||
module SketchupSuggestions | ||
# Avoid kernel `sleep` as it freezes up SketchUp. |
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 think "freezes SketchUp" is a more concise way to put the first part.
It's the "callback for the resource you are waiting for" part I'm unsure about. Is "callback for" better than "callback for"?
Add suggestion cop against
sleep
as it freezes up SketchUp.Mostly unsure how to best phrase the messages/documentation.