Kiwi image editor: Improve #to_xml to decrease the resulting diff #4002

ChrisBr · 2017-10-12T12:03:55Z

Only write the default package group
Write package group & repositories on same position as before

codecov · 2017-10-12T13:12:24Z

Codecov Report

Merging #4002 into master will decrease coverage by 0.02%.
The diff coverage is 8.33%.

@@            Coverage Diff            @@
##           master   #4002      +/-   ##
=========================================
- Coverage   89.22%   89.2%   -0.03%     
=========================================
  Files         309     309              
  Lines       18228   18235       +7     
=========================================
+ Hits        16264   16266       +2     
- Misses       1964    1969       +5

Flag	Coverage Δ
#api	`80.46% <8.33%> (-0.03%)`	⬇️
#rspec	`69.79% <ø> (ø)`	⬆️
#webui	`61.75% <8.33%> (-0.02%)`	⬇️

codecov · 2017-10-12T13:12:35Z

Codecov Report

Merging #4002 into master will decrease coverage by 0.06%.
The diff coverage is 95.12%.

@@            Coverage Diff             @@
##           master    #4002      +/-   ##
==========================================
- Coverage   89.18%   89.11%   -0.07%     
==========================================
  Files         309      310       +1     
  Lines       18236    18416     +180     
==========================================
+ Hits        16264    16412     +148     
- Misses       1972     2004      +32

Flag	Coverage Δ
#api	`81.13% <0%> (+0.75%)`	⬆️
#rspec	`69.97% <100%> (+0.07%)`	⬆️
#webui	`61.22% <0%> (-0.44%)`	⬇️

evanrolfe · 2017-10-13T08:30:15Z

For codecov, you can try change the class definition from this:

class Kiwi::Image

to this:

module Kiwi
  class Image

I think that might be why codecov isn't working for these files.. Also make the same change to Kiwi::PackageGroup

evanrolfe · 2017-10-13T08:37:21Z

src/api/app/models/kiwi/image.rb


-    # Reparser for pretty printing
-    Nokogiri::XML(doc.to_xml, &:noblanks).to_xml
+    Nokogiri::XML(doc.to_xml, &:noblanks).to_xml(indent: kiwi_indentation(doc))


Just wondering, would it be possible to use a .xml.builder view to generate this xml? Instead of Nokogiri and xpath? I'm finding it quite hard to read this code..

Adding the indent parameter to doc.to_xml wouldn't have the same effect?

@bgeuken would need to try that!

@evanrolfe Which part do you mean? I doesn't seem that complicated.

The whole method is hard to understand IMO, there are too many things going on. Its doing two things:

Deciding which things should go in the XML document.

Deciding how the XML document should be structured.

I think we should separate those two concerns just like how we separate models from views.

You are right. I think we should move some code to separate methods, like setting kiwi_body.

Moved to a dedicated Kiwi::Image::ToXml class!

evanrolfe · 2017-10-13T08:45:02Z

src/api/app/models/kiwi/image.rb

@@ -157,6 +158,15 @@ def self.binaries_available(project, use_project_repositories, repositories)

  private

+  def kiwi_indentation(xml)


Not sure if I really like having this in the model here. It looks like view logic to me.. I think we should start to think about seperating the business logic about kiwi images from the view logic about xml output. At the very least we could extract the xml presentation stuff into its own class like Kiwi::Image::ToXml or Kiwi::Image::XmlFormatter etc.

ChrisBr · 2017-10-13T08:47:47Z

@evanrolfe see #4010

mdeniz

Overall LGTM

mdeniz · 2017-10-13T08:49:31Z

src/api/app/models/kiwi/image.rb

+    # for now we only write the default package group
+    xml_packages = default_package_group.to_xml
+    packages = doc.xpath('image/packages[@type="image"]').first
+    packages ? packages.replace(xml_packages) : image.last_element_child.after(xml_packages)


Why not an if ... else ... end ?? This is a little bit hard to understand because is not used to assign the value to anything and this is running methods instead.

Is this explicitly tested?

Done!
Yes, I use one time the DEFAULT_KIWI_BODY (without packages and repositories) and the one time the kiwi_xml provided from include_context 'a kiwi image xml' which already contains packages and repositories!

mdeniz · 2017-10-13T08:51:36Z

src/api/app/models/kiwi/image.rb


+    previous = doc.xpath("image/repository").first.try(:previous) || image.last_element_child


I prefer to use another name for this variable... something like repository_position because in the case of the image.last_element_child this is not the previous node of anything.

Is this explicitly tested?

mdeniz · 2017-10-13T08:52:53Z

src/api/app/models/kiwi/image.rb

@@ -157,6 +158,15 @@ def self.binaries_available(project, use_project_repositories, repositories)

  private

+  def kiwi_indentation(xml)


You are basing this calculation in the space between the image tag and the first child inside it... what if this is not well indented? 🤔

Well, this is best effort. If the file is not well indented, we don't have any chance to fix it, right?

Maybe calculating the average of the indentation could be more closer... but anyway, is up to you to do that effort.

mdeniz · 2017-10-13T08:54:14Z

src/api/app/models/kiwi/image.rb

@@ -157,6 +158,15 @@ def self.binaries_available(project, use_project_repositories, repositories)

  private

+  def kiwi_indentation(xml)
+    content = xml.xpath('image').children.first.try(:content)
+    if content


In this case I would use the ternary operator ?, because you are returning a value, no running a method like in my other comment about using an if else end

in kiwi editor. Because for now we only allow to edit the default package group. Writing all package groups always would create a big diff.

to minimize the diff.

to minimize the generated diff.

ChrisBr · 2017-10-16T15:12:09Z

@mdeniz @evanrolfe @DavidKang @bgeuken Please review again

Extended the specs
Introduced dedicated ToXml class

Also talked with @adrianschroeter about the indentation: We will keep reading the first element for now. Introducing a diff without whitespace would be complicated and also introduce some new problems (e.g. when using which diff and also possible introduction of not wanted errors). Also hardcoding is not wanted. If the first element is wrong indented, the user has to deal with it for now!

evanrolfe · 2017-10-18T09:17:53Z

src/api/app/models/kiwi/to_xml.rb

@@ -0,0 +1,89 @@
+# TODO: Please overwrite this comment with something explaining the model target
+module Kiwi
+  class ToXml


[Nitpick] Minor point but can you make it clear that this is for converting a Kiwi::Image to xml? i.e. like:

module Kiwi class Image class ToXml

or

module Kiwi class ImageToXml

Bad naming (using a verb + a noun), I would prefer something like Kiwi::Image::Xml and instead of a class I also prefer to use a module to include in the Kiwi::Image class. That way you don't need to have the to_xml twice and also no need to instanciate an object.

If you stil prefer a class then use class methods, no need to create an instance of a class without any internal state, just the image reference, also because you aren't reusing this class in any way.

And also you will need to test this module/class alone.

For me I prefer to use a class to model complex operations like this instead of a module with methods. I dont really see a problem with having to instantiate an instance of ToXml..

IMO a class is a much more powerful tool for modelling the behaviour of Image#to_xml than a set of methods in a module.

Agreed on the naming! Either module or class I don't have a strong preference.
@mdeniz regarding the tests: atm this class is already implicit tested by the image specs. We can refactor this in a later PR.

How many times it is going to be used codewise? just one... no need to instanciate an object if you don't want to use the internal state too. I don't like it as it is.

How many times it is going to be used codewise? just one...

I don't see how that makes any difference?

no need to instanciate an object if you don't want to use the internal state too.

This is a class which outputs xml based on a kiwi image so the internal state is the kiwi image. Do you really think that putting a bunch of methods into a file is better than having a class to model behaviour? Are we Ruby developers or C developers? :P

regarding the tests: atm this class is already implicit tested by the image specs.

Yeah I agree. We are testing the behaviour of Kiwi::Image overall, so the tests shouldn't care if this code is in a class or a module or whatever..

I don't see how that makes any difference?

This is a class which outputs xml based on a kiwi image so the internal state is the kiwi image. Do you really think that putting a bunch of methods into a file is better than having a class to model behaviour?

I'm not talking about using a module but at least using a class method instead of an instance method, if the internall state is not changing why do we need an instance for it? is just a decorator method... so no need to have an object living for exporting one xml string based on the image.

Are we Ruby developers or C developers? :P

I'm a developer... not tied to any language 🗡

if the internall state is not changing why do we need an instance for it?

Because you can write code that is much more readable and easier to work with if you consider that the image is the internal state. The fact that it never changes doesn't matter.

so no need to have an object living for exporting one xml string based on the image.

Yes you're right theres no need to have an object for this, you can do it just fine with a method in a module or a class method, but the code will be much better if you use a object and instance methods for this. If you can save then state to a instance variable then that means you don't have to always pass the image around from one method to another.

Here are more reasons why we should not use a class method for this. Before you say "but you can find a blog post on the internet to support any claim you have about programming!", I tried to find anything supporting class methods for this kind of thing but could not..

http://blog.codeclimate.com/blog/2012/11/14/why-ruby-class-methods-resist-refactoring/
http://old.mlomnicki.com/programming/ruby/2011/07/20/class-vs-instance-methods.html

I'm a developer... not tied to any language 🗡️

Good answer 😄

Ok, anyway. At least change that name... is horrible 😄

evanrolfe

LGTM. One nitpick but otherwise I think it looks much better now that the xml stuff is in its own class, thanks!

mdeniz · 2017-10-18T09:30:34Z

src/api/app/models/kiwi/to_xml.rb

@@ -0,0 +1,89 @@
+# TODO: Please overwrite this comment with something explaining the model target
+module Kiwi
+  class ToXml


Bad naming (using a verb + a noun), I would prefer something like Kiwi::Image::Xml and instead of a class I also prefer to use a module to include in the Kiwi::Image class. That way you don't need to have the to_xml twice and also no need to instanciate an object.

If you stil prefer a class then use class methods, no need to create an instance of a class without any internal state, just the image reference, also because you aren't reusing this class in any way.

And also you will need to test this module/class alone.

mdeniz · 2017-10-18T09:35:56Z

src/api/spec/models/kiwi/image_spec.rb

+
+    context 'with a kiwi file with packages and repositories' do
+      let(:package) { create(:package) }
+      let(:kiwi_image) { Kiwi::Image.build_from_xml(kiwi_xml, 'some_md5') }


Does it make sense to have a factory for it?

mdeniz · 2017-10-18T09:36:03Z

src/api/spec/models/kiwi/image_spec.rb

+
+    context 'with a kiwi file without packages and repositories' do
+      let(:package) { create(:package) }
+      let(:kiwi_image) { Kiwi::Image.build_from_xml(Kiwi::Image::DEFAULT_KIWI_BODY, 'some_md5') }


Does it make sense to have a factory for it?

mdeniz · 2017-10-18T09:38:25Z

src/api/app/models/kiwi/to_xml.rb

+        return nil unless kiwi_file
+        @image.package.source_file(kiwi_file)
+      else
+        Kiwi::Image::DEFAULT_KIWI_BODY


What about a guard clause instead?

def kiwi_body return Kiwi::Image::DEFAULT_KIWI_BODY unless @image.package kiwi_file = @image.package.kiwi_image_file return nil unless kiwi_file @image.package.source_file(kiwi_file) end

evanrolfe · 2017-10-19T09:29:20Z

src/api/app/models/kiwi/image/xml.rb

+      #### To define class methods as private use private_class_method
+      #### private
+
+      #### Instance methods (public and then protected/private)


Can you get rid of these comments because this is not an ActiveRecord model so they don't make sense here..

does not alter the xml to decrease the resulting diff.

mdeniz

[nitpick] Does it have more sense to ename Kiwi::Image::Xml#to_xml method as to_s ??

evanrolfe reviewed Oct 13, 2017

View reviewed changes

evanrolfe suggested changes Oct 13, 2017

View reviewed changes

mdeniz suggested changes Oct 13, 2017

View reviewed changes

DavidKang added the Frontend Things related to the OBS RoR app label Oct 13, 2017

ChrisBr added 3 commits October 16, 2017 17:05

[webui] Only replace default package group

0e7f494

in kiwi editor. Because for now we only allow to edit the default package group. Writing all package groups always would create a big diff.

[webui] Add repositories after the first repository

71195a3

to minimize the diff.

[webui] Parse and use correct indentation for kiwi files

88d18be

to minimize the generated diff.

ChrisBr force-pushed the bugfix/kiwi_diff branch from b29bfcc to ff420c0 Compare October 16, 2017 15:06

evanrolfe reviewed Oct 18, 2017

View reviewed changes

evanrolfe approved these changes Oct 18, 2017

View reviewed changes

mdeniz suggested changes Oct 18, 2017

View reviewed changes

ChrisBr force-pushed the bugfix/kiwi_diff branch from ff420c0 to 6591753 Compare October 19, 2017 07:50

evanrolfe reviewed Oct 19, 2017

View reviewed changes

ChrisBr added 2 commits October 19, 2017 11:57

[webui] Move Kiwi::Image#to_xml to dedicated class

97542fa

[ci] Add specs for Kiwi::Image#to_xml

a12a5be

does not alter the xml to decrease the resulting diff.

ChrisBr force-pushed the bugfix/kiwi_diff branch from 22f4adb to a12a5be Compare October 19, 2017 09:57

mdeniz approved these changes Oct 19, 2017

View reviewed changes

mdeniz merged commit 051c111 into openSUSE:master Oct 19, 2017

ChrisBr deleted the bugfix/kiwi_diff branch February 21, 2018 13:07

		@@ -157,6 +158,15 @@ def self.binaries_available(project, use_project_repositories, repositories)

		private

		def kiwi_indentation(xml)


		previous = doc.xpath("image/repository").first.try(:previous) \|\| image.last_element_child

Kiwi image editor: Improve #to_xml to decrease the resulting diff #4002

Kiwi image editor: Improve #to_xml to decrease the resulting diff #4002

Conversation

ChrisBr commented Oct 12, 2017

codecov bot commented Oct 12, 2017

Codecov Report

codecov bot commented Oct 12, 2017 • edited Loading

Codecov Report

evanrolfe commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisBr commented Oct 13, 2017

mdeniz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisBr commented Oct 16, 2017

evanrolfe Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanrolfe Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanrolfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdeniz left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2017 •

edited

Loading

evanrolfe Oct 18, 2017 •

edited

Loading

evanrolfe Oct 18, 2017 •

edited

Loading

mdeniz left a comment •

edited

Loading