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

Delegated methods after `private` keyword are meant to be private #6819

Merged
merged 3 commits into from Mar 8, 2018

Conversation

Projects
None yet
5 participants
@ashmaroli
Member

ashmaroli commented Mar 1, 2018

Context:

Methods defined via def_delegator are public even if there is a preceding private keyword

Example:

private
def_delegator :@obj, :data, :fallback_data

Proof:

$ irb
require 'jekyll'
# => true

Jekyll::Drops::DocumentDrop.private_instance_methods.include?(:fallback_data)
# => false

Jekyll::Drops::DocumentDrop.public_instance_methods.include?(:fallback_data)
# => true
@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Mar 1, 2018

Not sure about StaticFileDrop though..:

class StaticFileDrop < Drop
extend Forwardable
def_delegators :@obj, :name, :extname, :modified_time, :basename
def_delegator :@obj, :relative_path, :path
def_delegator :@obj, :data, :fallback_data
def_delegator :@obj, :type, :collection
end

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Mar 1, 2018

What's the preferred style..?

class Foo
  extend Forwardable
  [...]

  private 

  def_delegator :@obj, :data, :fallback_data
  def_delegator :@obj, :path, :abs_path
  private :fallback_data, :abs_path

end

or

class Foo
  extend Forwardable

  def_delegator :@obj, :relative_path, :path
  private def_delegator :@obj, :data, :fallback_data
  private def_delegator :@obj, :path, :abs_path

  [...]
end

@oe oe added this to the v3.8.0 milestone Mar 1, 2018

@oe oe added the fix label Mar 1, 2018

@ashmaroli ashmaroli changed the title from WIP: Hide delegated methods after `private` keyword to Delegated methods after `private` keyword are meant to be private Mar 4, 2018

@ashmaroli ashmaroli requested review from parkr and benbalter Mar 4, 2018

@DirtyF DirtyF requested a review from jekyll/core Mar 4, 2018

@DirtyF

DirtyF approved these changes Mar 4, 2018

@oe

oe approved these changes Mar 4, 2018

@benbalter

Nice catch.

@DirtyF

This comment has been minimized.

Member

DirtyF commented Mar 8, 2018

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 297371e into jekyll:master Mar 8, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Mar 8, 2018

@oe oe referenced this pull request Mar 8, 2018

Closed

Release 3.8.0 #6783

12 of 13 tasks complete

@ashmaroli ashmaroli deleted the ashmaroli:private-delegated-methods branch Mar 9, 2018

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