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

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

Merged
merged 7 commits into from Jan 20, 2018

Conversation

Projects
None yet
5 participants
@parkr
Copy link
Member

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 and others added some commits Oct 24, 2017

@parkr parkr requested a review from jekyll/stability Dec 8, 2017

@ghost

ghost approved these changes Dec 8, 2017

@pathawks

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

ashmaroli commented Dec 8, 2017

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

👍

@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Dec 8, 2017

Replaced remaining p and puts with jekyll.logger

@DirtyF

DirtyF approved these changes Dec 8, 2017

@parkr
Copy link
Member Author

parkr left a comment

Thanks for your help, @DirtyF!

@@ -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

This comment has been minimized.

@parkr

parkr Dec 8, 2017

Author Member

We might have to call prog.to_s here.

This comment has been minimized.

@DirtyF

DirtyF Dec 8, 2017

Member

done

This comment has been minimized.

@ashmaroli

ashmaroli Dec 8, 2017

Member

LogAdapter converts all parameters to strings..

# 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

elsif prog.has_command? cmd
puts prog.commands[cmd]
Jekyll.logger.info prog.commands[cmd]

This comment has been minimized.

@parkr

parkr Dec 8, 2017

Author Member

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

This comment has been minimized.

@DirtyF

DirtyF Dec 8, 2017

Member

done

module Cop
module Jekyll
class NoPAllowed < Cop
MSG = "Avoid using p to print things".freeze

This comment has been minimized.

@parkr

parkr Dec 8, 2017

Author Member

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

@@ -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 g
Jekyll.logger.info g

This comment has been minimized.

@parkr

parkr Dec 8, 2017

Author Member

This should be just removed. It's debugging.

@DirtyF

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

ashmaroli commented Dec 11, 2017

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

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

This comment has been minimized.

Copy link
Member

DirtyF commented Jan 20, 2018

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 3f4bb55 into master Jan 20, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP ready for review
Details

@jekyllbot jekyllbot deleted the custom-cops-for-putsing branch Jan 20, 2018

jekyllbot added a commit that referenced this pull request Jan 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment