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

Add a DisableComment Cop #18842

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koddsson
Copy link

@koddsson koddsson commented Nov 28, 2024

Hey 👋🏻!

I wrote this rule at work where we have a very similar codebase and @MikeMcQuaid suggested that it could be useful in Homebrew as well so I essentially just copied it over here.

The rule enforces that any robocop:disable have a preceding comment that explains why the rule was disabled. This can be very useful when things are disabled for seemingly obvious reasons, but they become less and less obvious over time.

Everything looks good except for when I try to run the test I wrote, the test runner doesn't seem to find the test file 🤔. See the output log below.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
I feel like I'm probably holding it wrong?
 ~/src/koddsson/brew/Library/Homebrew/ [rubocops/disable_comment] brew tests --only=rubocops/disable_comment
Usage: brew tests [options]

Run Homebrew's unit and integration tests.

      --coverage                   Generate code coverage reports.
      --generic                    Run only OS-agnostic tests.
      --online                     Include tests that use the GitHub API and
                                   tests that use any of the taps for official
                                   external commands.
      --debug                      Enable debugging using ruby/debug, or
                                   surface the standard odebug output.
      --changed                    Only runs tests on files that were changed
                                   from the master branch.
      --fail-fast                  Exit early on the first failing test.
      --only                       Run only test_script_spec.rb. Appending
                                   :line_number will start at a specific
                                   line.
      --profile                    Run the test suite serially to find the n
                                   slowest tests.
      --seed                       Randomise tests with the specified value
                                   instead of a random seed.
  -d                               Display any debugging information.
  -q, --quiet                      Make some output more quiet.
  -v, --verbose                    Make some output more verbose.
  -h, --help                       Show this message.

Error: Invalid usage: The `--only` argument requires a valid file or folder name!

@koddsson
Copy link
Author

Evidently, something failed when I ran brew style as well 🤔

Good to see that the rule is working though!

@koddsson koddsson marked this pull request as draft November 28, 2024 16:08
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Dec 20, 2024
@MikeMcQuaid
Copy link
Member

Not stale, I'll help @koddsson with this some more after the holidays.

@github-actions github-actions bot removed the stale No recent activity label Dec 20, 2024
Copy link
Member

@issyl0 issyl0 left a comment

Choose a reason for hiding this comment

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

Thank you! It works, as evidenced by the syntax check failing because a lot of our disables don’t have comments (yet). I’ll take a bit of time over 🎄 to add comments in a separate PR, since you probably don’t want to acquire the context to do that @koddsson. 😅

I’ve added some “nit” comments and some Sorbet type sigs in this review.

For the problems you were having running the test, it should work if you cd $(brew --repo); gh pr checkout 18842; brew tests --only=rubocops/disable_comment.

end
end

it "register a offencse" do
Copy link
Member

Choose a reason for hiding this comment

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

(nit)

Suggested change
it "register a offencse" do
it "registers an offense" do

RUBY
end

it "doesn't register an offencse" do
Copy link
Member

Choose a reason for hiding this comment

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

(nit)

Suggested change
it "doesn't register an offencse" do
it "doesn't register an offense" do

class DisableComment < Base
MSG = "Add a clarifying comment to the RuboCop disable comment"

def on_new_investigation
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def on_new_investigation
sig { void }
def on_new_investigation

def disable_comment?(comment)
comment.text.start_with? "# rubocop:disable"
end

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sig { params(line: String).returns(T::Boolean) }


private

def disable_comment?(comment)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def disable_comment?(comment)
sig { params(comment: Parser::Source::Comment).returns(T::Boolean)
def disable_comment?(comment)

@@ -0,0 +1,32 @@
# typed: strict
Copy link
Member

Choose a reason for hiding this comment

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

As this is Sorbet typed: strict, I’ve added type signatures in-line.

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.

3 participants