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

Unleash::Context#to_s displays all properties #175

Merged
merged 2 commits into from
Jun 22, 2024

Conversation

nissyi-gh
Copy link
Contributor

About the changes

  • Refactored the to_s method in the Context class to dynamically include all instance variables.
  • This change ensures that any new instance variables added in the future will automatically be included in the string representation without needing to modify the to_s method.
  • This makes the method more flexible and maintainable.

RSpec and RuboCop were executed, and no issues were found related to this change.

Closes #172

Important files

  • lib/unleash/context.rb

Discussion points

  • None at this time.

- Refactored the `to_s` method in the `Context` class to dynamically include all instance variables.
- This change ensures that any new instance variables added in the future will automatically be included in the string representation without needing to modify the `to_s` method.
- This makes the method more flexible and maintainable.
Copy link
Collaborator

@rarruda rarruda left a comment

Choose a reason for hiding this comment

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

I would have preferred a simpler approach, without meta-programming.

Also, as in all other classes in the entire project, we do not use meta programming when writing the to_s method, and I would like to stick to that standard, instead of having different implementations in different classes.

I would however be positive to having meta programming in a test to verify that all instance variables are present in the to_s method, if you feel like doing that.

Or if using meta programming, leverage the built-in to_h built in method (as in the suggestion below).

Comment on lines 22 to 26
formatted_attributes = instance_variables.map do |var|
"#{var.to_s.delete('@')}=#{instance_variable_get(var)}"
end

"<Context: #{formatted_attributes.join(', ')}>"
Copy link
Collaborator

@rarruda rarruda Jun 21, 2024

Choose a reason for hiding this comment

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

If one were to use meta-programming, i'd have preferred if the built in to_h method was used instead:

Suggested change
formatted_attributes = instance_variables.map do |var|
"#{var.to_s.delete('@')}=#{instance_variable_get(var)}"
end
"<Context: #{formatted_attributes.join(', ')}>"
"<Context: " + self.to_h.map{|k,v| "#{k}=#{v}"}.join(',') + ">"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had no idea to use the to_h method. It is a very nice and simple implementation!

lib/unleash/context.rb Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 21, 2024

Pull Request Test Coverage Report for Build 9600614573

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 97.131%

Totals Coverage Status
Change from base Build 9594960976: 0.002%
Covered Lines: 2539
Relevant Lines: 2614

💛 - Coveralls

@nissyi-gh
Copy link
Contributor Author

@rarruda Thank you for your review!

I learned that I should refer to implementations in other classes. I also felt that a simpler implementation without meta-programming would be better.

Like the other classes, it is simple to implement and easy to understand.

Co-authored-by: Renato Arruda <[email protected]>
@nissyi-gh
Copy link
Contributor Author

I have confirmed once again that the RSpec and RuboCop checks pass.

@coveralls
Copy link

coveralls commented Jun 22, 2024

Pull Request Test Coverage Report for Build 9624141809

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.129%

Totals Coverage Status
Change from base Build 9594960976: 0.0%
Covered Lines: 2537
Relevant Lines: 2612

💛 - Coveralls

Copy link
Collaborator

@rarruda rarruda left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and for making the unleash gem better!

@rarruda rarruda merged commit e7c3b9e into Unleash:main Jun 22, 2024
20 checks passed
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.

Unleash::Context#to_s does not display all properties
3 participants