rubocop / rubocop-md

RuboCop for Markdown code snippets
MIT License
73 stars 14 forks source link

Implement cop disabling #36

Open ocvit opened 6 months ago

ocvit commented 6 months ago

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

# rubocop:enable all

def blank_method
^^^^^^^^^^^^^^^^ Style/EmptyMethod: Put empty method definitions on a single line.
end

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

````md
```ruby
# rubocop:disable all
def blank_method
end
# rubocop:enable all
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

<--rubocop/md-->

<--rubocop/md-->```ruby

def blank_method end

<--rubocop/md-->```

<--rubocop/md-->

<--rubocop/md--><span style=\"display:none;\"># rubocop:enable all


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:

https://github.com/rubocop/rubocop-md/blob/8ade43691b668a48a8cd68d43e32434da33d7e27/lib/rubocop/markdown/rubocop_ext.rb#L82-L86

```ruby
> 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:


I know that @Earlopain works on revised templating in https://github.com/rubocop/rubocop-md/pull/30 and it might change a lot of things underneath, but at least it'll work for now.

Earlopain commented 6 months ago

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 commented 6 months ago

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

Earlopain commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

@palkan @Earlopain any resolution on this?

palkan commented 5 months ago

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
...
...
...
Earlopain commented 5 months ago

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.

ocvit commented 5 months ago

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