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

Rubocop markdown snippets #47186

Merged
merged 1 commit into from Mar 25, 2023
Merged

Rubocop markdown snippets #47186

merged 1 commit into from Mar 25, 2023

Conversation

zzak
Copy link
Member

@zzak zzak commented Jan 30, 2023

Introduce rubocop-md for our markdown files.

This ensures any code snippets in the guides also follow the Rails coding conventions, without having to manually review every change that comes in.

I've intentionally disabled Style/StringLiterals in the guides because that is a much larger change, and figured I would follow up with a single PR to do that after this gets merged.

You can see some of the changes broken down further in zzak#2 and zzak#3.

@rails-bot rails-bot bot added the docs label Jan 30, 2023
@matthewd
Copy link
Member

I like the idea, though there are some specifics that don't seem great (attr_reader, %Q) -- being documentation examples, it can be better for us to use more generic constructs than optimise for the actual [dummy] content that's present as shown.

I also spotted a rubocop:disable comment somewhere.. does that show up in the generated docs?

@zzak
Copy link
Member Author

zzak commented Jan 31, 2023

Reverted some of the extraneous rules in 3ffc441.

I also spotted a rubocop:disable comment somewhere.. does that show up in the generated docs?

It did show up, so I reverted that as well. In spite of the docs providing an alternative method (<span style="display:none;"># rubocop:disable all</span>) that did not work when specifying this specific rule.. so probably better to just disable it.

We probably shouldn't worry about redundant begin/end in our docs 😂 and fine if we have to catch that one in code review IMO.

@@ -1073,7 +1073,7 @@ Passing an array of symbols is also acceptable.

```ruby
class Book
include ActiveModel:Validations
include ActiveModel::Validations
Copy link
Member Author

Choose a reason for hiding this comment

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

Regardless of the style consistency wins, the fact this does a Ripper.sexp to check the syntax of the code caught this which is a huge benefit IMO.

@zzak zzak force-pushed the rubocop-md branch 2 times, most recently from 71271d8 to cf67c1e Compare February 2, 2023 04:23
Comment on lines 275 to 276
routing (/^save@/i => :forwards)
routing (/@replies\./i => :replies)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a syntax error because of the space between routing and (.

If I'm correct, I'm surprised it isn't caught by RuboCop, especially given #47186 (comment)... 🤔

Was this autocorrected?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sambostock Thanks for taking a look!

This is not a syntax error, although I would prefer routing(...), it doesn't seem like there is a rule for that at least in our config.

>> RUBY_VERSION
=> "3.3.0"
?> def foo bar
?>   puts bar
>> end
=> :foo
>>
>> foo "omg"
omg
=> nil
>> foo("omg")
omg
=> nil
>> foo ("omg")
omg
=> nil

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the syntax error that @sambostock tells is as follows:

With space

invalid syntax:

% ruby -vce 'routing (/^save@/i     => :forwards)'
ruby 3.2.0 (2022-12-25 revision a528908271) [x86_64-darwin19]
-e:1: void value expression
...ing (/^save@/i     => :forwards)
-e: compile error (SyntaxError)

No space

valid syntax:

% ruby -vce 'routing(/^save@/i     => :forwards)'
ruby 3.2.0 (2022-12-25 revision a528908271) [x86_64-darwin19]
Syntax OK

The same is true for Ruby 3.3.0-dev.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I will fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sambostock @koic Fixed in 321c8df, but I wonder why Ripper.sexp didn't catch this? Is there another rule we can add that would have caught it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, but maybe there is something wrong with the Parser gem. Unfortunately, the cause has not been determined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both Ripper and Parser recognize this syntax:

irb(main):003:0> Ripper.sexp('routing (/^save@/i     => :forwards)')
=> 
[:program,
   [:@ident, "routing", [1, 0]],
   [:args_add_block,
    [[:paren,
      [:case,
       [:regexp_literal, [[:@tstring_content, "^save@", [1, 10]]], [:@regexp_end, "/i", [1, 16]]],
       [:in, [:symbol_literal, [:symbol, [:@ident, "forwards", [1, 27]]]], nil, nil]]]],
    false]]]]

irb(main):006:0> Parser::CurrentRuby.parse('routing (/^save@/i     => :forwards)')
=> 
s(:send, nil, :routing,                                                      
  s(:begin,                                                                  
    s(:match_pattern,                                                
      s(:regexp,                                                     
        s(:str, "^save@"),                                           
        s(:regopt, :i)),                                             
      s(:sym, :forwards)))) 

But:

$ ruby -w -e 'routing (/^save@/i     => :forwards)'

-e:1: void value expression
...ing (/^save@/i     => :forwards)
-e: compile error (SyntaxError)

🤷‍♂️

guides/source/active_record_postgresql.md Show resolved Hide resolved
guides/source/active_record_postgresql.md Show resolved Hide resolved
guides/.rubocop.yml Outdated Show resolved Hide resolved
guides/.rubocop.yml Outdated Show resolved Hide resolved
guides/source/association_basics.md Outdated Show resolved Hide resolved
@zzak zzak force-pushed the rubocop-md branch 3 times, most recently from 0151e55 to 794bd7d Compare March 15, 2023 01:45
@zzak zzak requested a review from rafaelfranca March 15, 2023 02:33
@zzak zzak added the ready PRs ready to merge label Mar 15, 2023
@rafaelfranca rafaelfranca merged commit 06a8427 into rails:main Mar 25, 2023
zzak added a commit to zzak/rails that referenced this pull request Mar 27, 2023
This is a follow up to rails#47186, this time for all markdown content.

[markdownlint](https://github.com/markdownlint/markdownlint) is an excellent tool, and I've found it very useful for finding issues in the guides.

Many of the rules are common style issues I'm correcting on PRs, so it will be nice to have that automated.

We should also be able to use the same config with our editors, so that errors show up in real-time 🙏 and will update the contributing docs once this gets merged with how to debug and use mdl appropriately.
@zzak zzak deleted the rubocop-md branch March 27, 2023 05:54
okuramasafumi added a commit to okuramasafumi/alba that referenced this pull request Apr 5, 2023
toshimaru added a commit to toshimaru/rubocop-rails_config that referenced this pull request Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs ready PRs ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants