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
[Review] Request from 'aduffeck' @ 'SUSE/machinery/review_150701_improve_compare_output' #1061
Conversation
@@ -36,7 +42,7 @@ class BarBazScope < Machinery::Object | |||
end | |||
|
|||
class BarBazRenderer < Renderer | |||
def do_render | |||
def content(description) |
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.
Unused method argument - description
. If it's necessary, use _
or _description
as an argument name to indicate that it won't be used. You can also write as content(*)
if you want the method to accept any arguments but don't care about them.
Comparison#compare_scope returns a Comparison::result which can be used like that: comparison.name1 # name of description 1 comparison.name2 # name of description 2 comparison.only_in1 # partial scope of data only in description 1 comparison.only_in2 # partial scope of data only in description 2 comparison.changed # elements in both with changed attributes comparison.common # partial scope of data in both descriptions comparison.as_description(:one) # methods for returning the according comparison.as_description(:two) # data wrapped into a SystemDescription comparison.as_description(:common)
There is now a well defined set of "content*" methods that are supposed to be overriden in the subclasses. That makes the separation of the responsibilities clearer.
e45b437
to
7f97b2c
Compare
# you may find current contact information at www.suse.com | ||
|
||
class Comparison | ||
class Result |
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.
Is this inner class really needed? Couldn't the Comparison class itself have all the methods?
b8b2bbb
to
4cd6a68
Compare
lgtm |
[Review] Request from 'aduffeck' @ 'SUSE/machinery/review_150701_improve_compare_output'
Please review the following changes: