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

Fix default procs' behavior when overriding keywords in subclasses #5

Merged
merged 3 commits into from
Mar 22, 2021

Conversation

maxim
Copy link
Owner

@maxim maxim commented Mar 18, 2021

Portrayal relies on an ordered ruby hash to initialize keywords in the correct order. However, if overriding the same keyword in a subclass (by declaring it again), it didn't move keyword to the bottom of the hash, so this would happen:

class Person
  extend Portrayal
  keyword :email, default: nil
end

class Employee < Person
  keyword :employee_id
  keyword :email, default: proc { "#{employee_id}@example.com" }
end

employee = Employee.new(employee_id: '1234')
employee.email # => "@example.com"

The email is broken because it relies on having employee_id declared before email, but email was already declared first in the superclass. This change fixes situations like this by re-adding the keyword to the bottom of the hash on every re-declaration.

Additional commits in here:

  • Update github rspec workflow with Ruby 3, and non-deprecated action
  • Add readme section for subclassing

@maxim maxim requested review from cover and phillipgray March 18, 2021 14:38
Copy link
Collaborator

@cover cover left a comment

Choose a reason for hiding this comment

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

Looks great 👍

Copy link

@phillipgray phillipgray left a comment

Choose a reason for hiding this comment

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

Solid fix. 🚢

@maxim maxim merged commit fb5b221 into main Mar 22, 2021
@maxim maxim deleted the fix-procs-in-subclassing branch March 22, 2021 18:57
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.

None yet

3 participants