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

Allow to inherit one page from another #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Envek
Copy link
Member

@Envek Envek commented Nov 22, 2017

To allow reuse of common sections and to allow to add or remove sections for specific cases.

Have replaced an array of sections to hash of sections as it is easier to work with now and allows to avoid full array scan to check for duplicated sections.

Copy link
Member

@nepalez nepalez left a comment

Choose a reason for hiding this comment

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

In my opinion its better to extract definition for a section with one method:

#
# Definition of section
#
class Tram::Page::Section
  extend Dry::Initializer

  param  :name
  option :skip,   optional: true
  option :if,     optional: true, as: :positive
  option :unless, optional: true, as: :negative

  # Takes the page instance and extracts a hash for the section
  def call(page)
    show?(page) ? { name => value(page) } : {}
  end

  private

  def value(page)
    page.public_send name
  end

  def show?(page)
    condition  = !skip
    condition &= page.send(positive)  if positive
    condition &= !page.send(negative) if negative
    condition
  end
end

and use it to wrap args it in sections hash (in a subclass we'd just reload parent section with a new definition):

@sections[name] = Section.new(name, options)

in a runtime we just merge all section-provided hashes for the page:

self.class.sections.values.map { |section| section.call(self) }.reduce({}, :merge)

All we left to do is to copy section in the inherited class (like you do)

IMO extraction of definitions is a more powerful mechanism in case of inheritance/reloading, than messing inheritance with processing concrete settings (like :skip)

@gzigzigzeo
Copy link
Contributor

From one point of view, I agree with @nepalez: explicit mixing is better. In the another hand, you will need to do mixing in-place in each and every line of code where child page is called. Could we consider something like:

inherit_section :parent_section
?

@Envek
Copy link
Member Author

Envek commented Nov 24, 2017

@nepalez, well, I've tried to extract sections, but it's weird, as currently sections are expected to define methods in page itself, so I can't figure how to cleanly separate logic between the Tram::Page and Tram::Page::Section. See e4fbafb

@Envek
Copy link
Member Author

Envek commented Nov 24, 2017

@gzigzigzeo, from the one side it will make configuration of inherited pages much more verbose and it will require changing inherited pages every time parent page layout is changed.

But on the other hand, child page layout explicitness will prevent large unneeded sections from parent pages to unnoticeably leak into the child pages. And this is a good thing.

Implemented inherit_section.

To allow reuse of common sections and to allow to add
or remove sections for specific cases.
@Envek
Copy link
Member Author

Envek commented Nov 26, 2017

@nepalez, rebased on top of current master.

option :if, proc(&:to_s), optional: true, as: :positive
option :unless, proc(&:to_s), optional: true, as: :negative
option :skip, true.method(:&), optional: true
option :method, proc(&:to_s), default: -> { name }
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd better rename these methods because the original ones can provide problems during debugging
(but I agree to not renaming value to block)

Copy link
Member

Choose a reason for hiding this comment

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

Not to mention using attributes adds a bunch of excessive steps in a runtime

end

def value_at(page)
block ? page.instance_exec(&block) : page.public_send(method_name)
if attributes[:value]
Copy link
Member

Choose a reason for hiding this comment

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

value is already memoized, we don't need looking for it in a hash


class InheritedPage < ExamplePage
inherit_section :foo
inherit_section :bar, if: :no_bar
Copy link
Member

Choose a reason for hiding this comment

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

I think this DSL breaks the Less Surprise Principle. When I inherit a page from another one, I expect the children to have all the parent's sections (sections is the main responsibility for Tram::Page) without necessity to explicitly select them.

What I'd like is something like:

class InheritedPage < ExamplePage
  # section :foo is already here from `ExamplePage`
  section :bar, if: :no_bar, reload: true # tells explicitly: "I know that :bar is already here, and yes, I want to reload it"
  section :bam
end

@gzigzigzeo ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants