Skip to content

Fix inheritance sample code in CONCEPTS.md #35

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −8
Open

Fix inheritance sample code in CONCEPTS.md #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions CONCEPTS.ja.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,12 +297,14 @@ ComputedModelで部分的にフィールドを定義したクラス (モジュ

```ruby
module UserLikeConcern
extends ActiveSupport::Concern
extend ActiveSupport::Concern
include ComputedModel::Model

dependency :preference, :profile
computed def display_name
"#{preference.title} #{profile.name}"
included do
Copy link
Collaborator

@qnighy qnighy Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is not an intended usage. Is there a problem with computed_model itself? If it's pb-serializer's problem, please leave it as-is and write a notice somewhere else.

Copy link
Collaborator

@qnighy qnighy Jul 8, 2021

Choose a reason for hiding this comment

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

Although include do ... end may work as well, this is unrelated to computed_model's inheritance feature, so the example won't fit here.

Copy link
Contributor Author

@izumin5210 izumin5210 Jul 8, 2021

Choose a reason for hiding this comment

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

This seems to be a problem on the computed model side.
Here is the code that reproduces it. If you do not use included, you will get an error.

module UserConcern
  extend ActiveSupport::Concern
  include ComputedModel::Model

  dependency raw_user: [:first_name, :last_name]
  computed def name
    [raw_user.first_name, raw_user.last_name].map(&:presence).compact.join(' ')
  end
end

# NoMethodError: undefined method `dependency' for UserConcern:Module
# from <main>:5:in `<module:UserConcern>'

Copy link
Collaborator

@qnighy qnighy Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, reproduced! It's strange though. Kinda ActiveSupport::Concern's corner case? The following modifications work.

module UserConcern
  # extend ActiveSupport::Concern
  include ComputedModel::Model

  dependency raw_user: [:first_name, :last_name]
  computed def name
    [raw_user.first_name, raw_user.last_name].map(&:presence).compact.join(' ')
  end
end
module UserConcern
  extend ActiveSupport::Concern
  include ComputedModel::Model
  extend ComputedModel::Model::ClassMethods

  dependency raw_user: [:first_name, :last_name]
  computed def name
    [raw_user.first_name, raw_user.last_name].map(&:presence).compact.join(' ')
  end
end

Copy link
Collaborator

@qnighy qnighy Jul 8, 2021

Choose a reason for hiding this comment

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

Anyway include do ... end is not an intended fix. Let's investigate it in #36

dependency :preference, :profile
computed def display_name
"#{preference.title} #{profile.name}"
end
end
end

Expand Down
10 changes: 6 additions & 4 deletions CONCEPTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,12 +303,14 @@ You can also define partial ComputedModel class/module. You can then inherit/inc

```ruby
module UserLikeConcern
extends ActiveSupport::Concern
extend ActiveSupport::Concern
include ComputedModel::Model

dependency :preference, :profile
computed def display_name
"#{preference.title} #{profile.name}"
included do
dependency :preference, :profile
computed def display_name
"#{preference.title} #{profile.name}"
end
end
end

Expand Down