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

Command title and description #167

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Eneroth3
Copy link
Contributor

No description provided.


require 'spec_helper'

describe RuboCop::Cop::SketchupSuggestions::CommandTitle do
Copy link
Member

Choose a reason for hiding this comment

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

To make this pass the tests we need to change this line to:

describe RuboCop::Cop::SketchupSuggestions::CommandTitle, :config do

And then remove:

subject(:cop) { described_class.new }

Copy link
Member

Choose a reason for hiding this comment

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

That is what was done to all our existing cops to fix the build:
679d2e2

end

# TODO: Add for setter methods.
# Can we test by how a local variable was defined? Or only by its name?
Copy link
Member

Choose a reason for hiding this comment

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

What do you want to test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking something like one node catcher than looks for {x} = UI::Command.new(...) for any x, and then a second search in the same local scope for {x}.menu_text =. Otherwise it can maybe just check for menu_text = and assume there is no other method by the same name.

text == title_case(text)
end

def title_case(text)
Copy link
Member

Choose a reason for hiding this comment

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

Worth a comment on how this differ from Ruby's own title case method.


private

# REVIEW: Extract to where they can be re-used?
Copy link
Member

Choose a reason for hiding this comment

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

Maybe in a lib\rubocop\sketchup\formatting.rb file? A RuboCop::SketchUp::Formatting mixing module?
Like lib\rubocop\sketchup\namespace_checker.rb?

@thomthom
Copy link
Member

thomthom commented May 2, 2023

Looking good. Did you want to address the REVIEW comment now, or leave it for later?

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