-
Notifications
You must be signed in to change notification settings - Fork 28
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
Remove OpenStruct usage with Struct replacement #53
Conversation
This is no longer necessary since we've replaced the single OpenStruct reference with a plain Struct instead.
Since #49 also introduced the json gem dependency, I believe that can be removed here since OpenStruct is no longer used. If desired, I can remove the gemspec dependency here. |
def to_struct | ||
OpenStruct.new(self.each_with_object({}) do |(key, val), acc| | ||
acc[key] = val.is_a?(Hash) ? val.to_struct : val | ||
end) | ||
Struct.new(*self.keys).new(*self.values.map { |value| value.is_a?(Hash) ? value.to_struct : value }) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if to_struct
is even needed. It converts the notification passed to the group_started
method. It looks like, RSpec::Core::Notifications::GroupNotification
can be used instead.
@smileart Do you remember why RSpec::Core::Notifications::GroupNotification
was not used instead in #7?
@@ -2,7 +2,6 @@ | |||
|
|||
require "open3" | |||
require "fileutils" | |||
require "ostruct" | |||
require "json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json
gem is used TurboTests::JsonRowsFormatter#JsonRowsFormatter
to provide to_json
method, similarly to the RSpec::Core::Formatters::JsonFormatter#close
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, indeed! Sounds like we still need json then!
Nice! I don't know the root cause of the ruby-core issue, but this direction feels like a nice regardless. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @javierjulio!
I'll merge this PR and address to_struct
removal later (ref: https://github.com/serpapi/turbo_tests/pull/53/files#r1567009884),
@ilyazub you're welcome and thank you! That sounds good if it's not needed then. |
This replacement to use a struct should still resolve the original issue in #49 but also with #49 (comment) for the reported ruby core issue.
Example you can try in an IRB console: