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

Implement cop disabling #36

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ocvit
Copy link

@ocvit ocvit commented Apr 26, 2024

Hi,

I've recently discovered that proposed way of disabling cops doesn't work:

<span style="display:none;"># rubocop:disable all</span>

```ruby
def blank_method
end
```

<span style="display:none;"># rubocop:enable all</span>
```ruby
def blank_method
^^^^^^^^^^^^^^^^ Style/EmptyMethod: Put empty method definitions on a single line.
end
```

It doesn't even work when used inside Ruby blocks:

```ruby
# rubocop:disable all
def blank_method
end
# rubocop:enable all
```
```ruby
def blank_method
^^^^^^^^^^^^^^^^ Style/EmptyMethod: Put empty method definitions on a single line.
end
```

So I started digging into a problem and found out that the reason why span way fails is because of missing implementation whatsoever. These blocks are just marked out as irrelevant lines:

#<--rubocop/md--><span style=\"display:none;\"># rubocop:disable all</span>
#<--rubocop/md-->
#<--rubocop/md-->```ruby
def blank_method
end
#<--rubocop/md-->```
#<--rubocop/md-->
#<--rubocop/md--><span style=\"display:none;\"># rubocop:enable all</span>

The trick here is to extract decorator itself, so it is treated as a Ruby code. Plus - add the same marker, so decorator can be restored to original span block in case of correction. Marker should be applied to the end of a line though:

# rubocop:disable all <--rubocop/md-->
#<--rubocop/md-->
#<--rubocop/md-->```ruby
def blank_method
end
#<--rubocop/md-->```
#<--rubocop/md-->
# rubocop:enable all <--rubocop/md-->

The reason why it doesn't work inside Ruby blocks is in fact that offense is never checked for #disabled status:

offenses_per_cop.flatten(1).reject do |offense|
next if RuboCop::Markdown::MARKDOWN_OFFENSE_COPS.include?(offense.cop_name)
offense.location.source_line.start_with?(marker_comment)
end

> offense
# => #<RuboCop::Cop::Offense:0x000000011e8b2260
#  @cop_name="Style/EmptyMethod",
#  @corrector=nil,
#  @location=#<Parser::Source::Range test.md 47...67>,
#  @message="Style/EmptyMethod: Put empty method definitions on a single line.",
#  @severity=#<RuboCop::Cop::Severity:0x000000011ec10e70 @name=:convention>,
#  @status=:disabled>  <----- !

This PR:

  • implements cop disabling using proposed span block with autocorrection support
  • fixes cop disabling inside Ruby blocks

I know that @Earlopain works on revised templating in #30 and it might change a lot of things underneath, but at least it'll work for now.

@Earlopain
Copy link
Contributor

I tested out a few versions and it seems to have been broken for a very long time already. I tested 1.2.0 and 1.0.0 (from 4 years ago).

If my PR goes through, the mechanism for disabling per code block will need to change anyways. Since it doesn't touch markdown anymore it also has no good idea about whichever comments may preceed or follow a codeblock.

What I came up with was something like this:

'' + ""

which is this:

```ruby norubocop
'' + ""
```

There would be no offenses in this block. GitHub and GitLab still correctly syntax highlight so I assume there is something in the spec about this.

So, since this is already broken anyways I think it would make sense to switch over to something like that. Let's hear what palkan thinks.

@ocvit
Copy link
Author

ocvit commented Apr 26, 2024

@Earlopain norubocop annotation looks really neat, way better than clunky span. Is there a variant for disabling specific cops only?

@Earlopain
Copy link
Contributor

I suppose you could tack on as much as you want to that line but I wonder how useful that would actually be. What is your usecase? In my mind, if you want to disable a cop for the entirey of a codeblock you'd be better to disable that cop alltogether or exclude that file specifically.

@ocvit
Copy link
Author

ocvit commented Apr 26, 2024

I encountered situation where I wanted to have partial descriptions in docs specifically, that are against the rules otherwise and shouldn't be used on a regular basis:

# fails, OK readability
Some::Resource.new("https://some_lengthy_url_that_breaks_readability",
  param_a: 1, # default: 0
  param_b: 2, # default: 10
  param_c: 3  # ...other description
) 

# correct way according to Rubocop, worse readability
Some::Resource.new(
  "https://some_lengthy_url_that_breaks_readability",
  param_a: 1, # default: 0
  param_b: 2, # default: 10
  param_c: 3  # ...other description
) 

Disabling Rubocop for the entirety of codeblock isn't optimal if you want to ignore specific offense but otherwise make sure that everything else has been checked:

```ruby norubocop
# you wanted to disable an offense on this line
%w[foo bar baz] * ","

# ...but you also disabled this one unintentionally
'' + ""
```

@ocvit
Copy link
Author

ocvit commented Apr 26, 2024

But honestly, it's not even about the usecase. The fact that it's technically configurable by rubocop means it should be able to be config'ed inside Markdown files as well.

@ocvit
Copy link
Author

ocvit commented May 2, 2024

@palkan @Earlopain any resolution on this?

@palkan
Copy link
Collaborator

palkan commented May 5, 2024

Hey @ocvit!

Thanks for the detailed explanation. I suggest extracting the offense.disabled? check into a separate PR (this is a bug fix, we can merge it quickly) and figure out the other part (disabling outside of the Ruby code) later.

it seems to have been broken for a very long time already

Yeah, unfortunately, we didn't have tests for this scenario (my bad).

The problem is that such span-ed comments are treated as single line comments, not range comments: https://github.com/rubocop/rubocop/blob/49c6e92964e87435af7fee6d87bc6c4012fc8f04/lib/rubocop/directive_comment.rb#L39

So, one option to workaround this is to patch the DirectiveComment class and make it aware of HTML comments in Markdown files (hacky, yes; but pretty straightforward and easy to implement).

Re: norubocop modifier.

I like it but I'm not sure how it would be treated by Markdown renderers; the syntax highlight syntax itself is an extension and I couldn't find any specification.

What would be the API supported by major Markdown renderers?


```ruby norubocop
...
```

```ruby+norubocop
...
``` 

```ruby # rubocop:disable
...
```

@Earlopain
Copy link
Contributor

Earlopain commented May 6, 2024

Re: norubocop modifier

As per CommonMark, the stuff after the codefence is called an infostring: https://spec.commonmark.org/0.31.2/#info-string

The content of a code fence is treated as literal text, not parsed as inlines. The first word of the info string is typically used to specify the language of the code sample, and rendered in the class attribute of the code tag. However, this spec does not mandate any particular treatment of the info string.

So, it's basically a "do whatever you want". Implementations that test against the CommonMark examples will handle this correctly since there is a testcase for this at https://spec.commonmark.org/0.31.2/spec.json:

grafik

I also found this neat website which shows the output of a bunch of markdown implementations: https://babelmark.github.io/?text=%60%60%60ruby+norubocop%0A%27%27+%2B+%22%22%0A%60%60%60%0A%0A%60%60%60ruby%0A%27%27+%2B+%22%22%0A%60%60%60 (takes a bit to load everything)

The results don't deviate much against markdown with just a language type so I would be confident so say that this is widely supported. Some output slightly unexpected results but will still render as expected, others don't support language hints at all. I'll come back to this comment when it is relevant, for now I'll link to this in the tracking issue.

@Earlopain Earlopain mentioned this pull request May 6, 2024
9 tasks
@ocvit
Copy link
Author

ocvit commented May 9, 2024

Hey @palkan, thanks for DirectiveComment suggestion. It really simplifies everything. PR has been updated.

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