8000 Write a Rubocop Cop to ensure no `#p` or `#puts` calls get committed to master. by parkr · Pull Request #6615 · jekyll/jekyll · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Write a Rubocop Cop to ensure no #p or #puts calls get committed to master. #6615

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 20, 2018
Merged

Write a Rubocop Cop to ensure no #p or #puts calls get committed to master. #6615

merged 7 commits into from
Jan 20, 2018

Conversation

parkr
Copy link
Member
@parkr parkr commented Dec 8, 2017

Using puts or p should be disallowed, in favor of always using Jekyll.logger. puts and p are useful for debugging, but should never land on the master branch.

In order to merge this, we need to fix a few instances of puts and p!

@parkr parkr requested a review from a team December 8, 2017 16:08
@pathawks
Copy link
Member
pathawks commented Dec 8, 2017

I love this conceptually, and presumably this would also become enabled in plugins that inherit Jekyll’s Rubocop config.

(Won’t have time to do an in-depth code review today, but I am a +1)

@parkr
Copy link
Member Author
parkr commented Dec 8, 2017

I love this conceptually, and presumably this would also become enabled in plugins that inherit Jekyll’s Rubocop config.

Oh, unclear actually. I think we'd have to add this directory to s.files in the gemspec!

@ashmaroli
Copy link
Member

I think we'd have to add this directory to s.files in the gemspec!

👍

@DirtyF
Copy link
Member
DirtyF commented Dec 8, 2017 •
edited
Loading

Replaced remaining p and puts with jekyll.logger

parkr
parkr commented Dec 8, 2017
View reviewed changes
Copy link
Member Author
@parkr parkr left a comment

Choose a reason for hiding this comment

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

Thanks for your help, @DirtyF!

lib/jekyll/commands/help.rb Outdated
@@ -12,9 +12,9 @@ def init_with_program(prog)
c.action do |args, _|
cmd = (args.first || "").to_sym
if args.empty?
puts prog
Jekyll.logger.info prog
Copy link
Member Author

Choose a reason for hiding this comment

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

We might have to call prog.to_s here.

Copy link
Member

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

LogAdapter converts all parameters to strings..

jekyll/lib/jekyll/log_adapter.rb

Lines 114 to 122 in 1691685

# Internal: Format the topic
#
# topic - the topic of the message, e.g. "Configuration file", "Deprecation", etc.
# colon -
#
# Returns the formatted topic statement
def formatted_topic(topic, colon = false)
"#{topic}#{colon ? ": " : " "}".rjust(20)
end

lib/jekyll/commands/help.rb Outdated
elsif prog.has_command? cmd
puts prog.commands[cmd]
Jekyll.logger.info prog.commands[cmd]
Copy link
Member Author

Choose a reason for hiding this comment

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

We might have to call to_s on prog.commands[cmd] here.

Copy link
Member

Choose a reason for hiding this comment

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

done

rubocop/jekyll/no_p_allowed.rb Outdated
module Cop
module Jekyll
class NoPAllowed < Cop
MSG = "Avoid using p to print things".freeze
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you use the same message as in no_puts_allowed that suggests Jekyll.logger instead?

test/test_filters.rb Outdated
@@ -779,7 +779,7 @@ def to_liquid
should "include the size of each grouping" do
grouping = @filter.group_by(@filter.site.pages, "layout")
grouping.each do |g|
p< 8000 /span> g
Jekyll.logger.info g
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be just removed. It's debugging.

@DirtyF
Copy link
Member
DirtyF commented Dec 8, 2017

added a puts in a plugin and tested this branch

output:

lib/jekyll-seo-tag/json_ld.rb:26:9: C: Jekyll/NoPutsAllowed: Avoid using puts to print things. Use Jekyll.logger instead.
        puts "Jekyll::SeoTag::JSONLD is deprecated"
        ^^^^

works fine!

@ashmaroli
Copy link
Member

this needs some tests.. especially one for the following scenario:

File.open("foo", "wb") do |f|
  f.puts heredoc
end

@DirtyF
Copy link
Member
DirtyF commented Jan 20, 2018

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 3f4bb55 into master Jan 20, 2018
@jekyllbot jekyllbot deleted the custom-cops-for-putsing branch January 20, 2018 22:04
jekyllbot added a commit that referenced this pull request Jan 20, 2018
@jekyllbot
Update history to reflect merge of #6615 [ci skip]
8ee052d
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DirtyF DirtyF DirtyF approved these changes

@ashmaroli ashmaroli ashmaroli left review comments

Assignees
No one assigned
Labels
fix frozen-due-to-age internal tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@parkr @pathawks @ashmaroli @DirtyF @jekyllbot
0