Command title and description #165
Conversation
|
|
||
| def on_send(node) | ||
| return unless init_entity?(node) | ||
| return if node.arguments.first.str_content.nil? |
There was a problem hiding this comment.
Is there a more readable way to see if an argument is a string token?
I want to catch method("some_string") but ignore method(some_variable).
This line gets the job done but is not very clear.
There was a problem hiding this comment.
Sounds like all string literal nodes will be RuboCop::AST::StrNode
https://rubydoc.info/github/rubocop/rubocop-ast/RuboCop/AST/StrNode
This appear to also include heredoc strings.
There was a problem hiding this comment.
That being said, you are using a node matcher only to query if the node is something - you can use it to also capture.
First I think we can replace ... with something that captures string literals: (str $_) (I'm not 100% sure about the syntax). This might also not capture all string literals, I'd have to dig deeper.
But notice we added (str into the pattern, making the pattern check the type instead of the outer logic. Then we have $ - it captures the node. You can see an example of this in SketchUpBugs/RenderMode. Also in SketchupRequirements/MinimalRegistration.
There was a problem hiding this comment.
There is also node.str_type? (SketchupSuggestions/FileEncoding)
|
|
||
| private | ||
|
|
||
| # REVIEW: Extract to where they can be re-used? |
There was a problem hiding this comment.
Do we have a good place to put these metho9ds for re-use between cops?
There was a problem hiding this comment.
For reusable logic I've added this as mix-in modules. See DynamicComponentGlobals, SketchUpTargetRange etc.
| end | ||
|
|
||
| # TODO: Add for setter methods. | ||
| # Can we test by how a local variable was defined? Or only by its name? |
There was a problem hiding this comment.
Can the node matcher be used to match a local variable based on its previous definition?
x = UI::Command.new("Bla bla") # Define a node matcher for this definition
x.menu_text = "Bla bla" # Match for any X, using "nested" node matcher
Or is our best bet to hardcode typical variable names?
There was a problem hiding this comment.
It is possible to track variable names, though you have to do that manually. Because of that I've never done anything more than check the local scope. I think I've done something like this in the cops for register_extension.
That might make sense. Probably cleaner to cop logic doing it like that. |
| end | ||
|
|
||
| def title_case(text) | ||
| text.gsub(/^(.)/) { ::Regexp.last_match(1).upcase } |
There was a problem hiding this comment.
Is this different from string.capitalize?
https://ruby-doc.org/core-2.7.1/String.html#method-i-capitalize
If it is, can you add a comment with examples of what it does? (Hard to tell from just glancing at the regexes)
|
Closing as the tests fail. |
|
@Eneroth3 - the failing tests were due to some changes in RubyCop 1.49 that flushed out issues in our own tests. They are fixed now. If you rebase you should see them pass again. |
Adding cop for checking that command titles are in title case.
Not sure if #145 should be split up and sentence case for the statusbar should be logged as a separate issue.