Add context to Autoinst issues #400

imobachgs · 2017-10-27T13:21:21Z

This PR adds some context to Autoinst issues adding the section where they were detected.

PartitioningSection, SectionsWithAttributes (and derived classes) and DriveSection have a reference to its parent.
Issues hold a reference to the section where the problem was found.
Some minor refactoring regarding issues lists tests was performed.

With those changes in place, AutoYaST will benefit when showing errors information to users.

What is missing?

Add the parent method to SkipListSection.
Improve sections tests, including one (possible shared) to check the parent method.

coveralls · 2017-10-30T13:02:52Z

Coverage decreased (-0.01%) to 95.56% when pulling c1b5e71 on imo-issues-context into 1ab5982 on master.

coveralls · 2017-10-30T22:18:07Z

Coverage increased (+0.002%) to 95.575% when pulling 6ce6595 on imo-issues-context into 1ab5982 on master.

jreidinger · 2017-10-31T08:10:19Z

src/lib/y2storage/autoinst_issues/invalid_value.rb

+      #
+      # @return [Integer,String,Symbol] Invalid value
+      def value
+        section.send(attr)


well, this allow to also call private methods, which is not so nice from my POV. public_send is not enough?

jreidinger · 2017-10-31T08:11:03Z

src/lib/y2storage/autoinst_issues/invalid_value.rb

+        # TRANSLATORS: 'value' is a generic value (number or string) 'attr' is an AutoYaST element
+        # name; 'new_value_message' is a short explanation about what should be done with the value.
+        _("Invalid value '%{value}' for attribute '%{attr}' (%{new_value_message}).") %
+          { value: value, attr: attr, new_value_message: new_value_message }


rubocop is fine with this? I think it usually suggest to use format, but maybe it is here more benevolent.

Yes, it is fine.

jreidinger · 2017-10-31T08:14:41Z

src/lib/y2storage/autoinst_issues/issue.rb

@@ -7,6 +7,9 @@ module AutoinstIssues
    class Issue
      include Yast::I18n

+      # @return [Object] section where the problem was detected (see {AutoinstProfile})


well, to be honest documenting only Object as parent is not much helpful. is there any methods on top of pure Object that it have in common? I think for documentation purpose it would be helpful to have class that define common methods for all sections.

currently it looks like at least parent and section_name is must have, not?

Well, that's partially true: these classes will only hold a reference to those objects (to be used by others later). But it's true that maybe something like #parent,#section_name would be better than Object.

jreidinger · 2017-10-31T08:19:00Z

src/lib/y2storage/autoinst_profile/section_with_attributes.rb

+        klass_name = self.class.name.split("::").last
+        klass_name
+          .gsub(/([a-z])([A-Z])/, "\\1_\\2").downcase
+          .gsub(/_section\z/, "")


gsub here is not needed, it can be simple sub or even better .chomp("_section")

jreidinger · 2017-10-31T08:21:56Z

test/support/autoinst_profile_examples.rb

@@ -0,0 +1,35 @@
+# encoding: utf-8


why file is called profile_examples, when it is section-examples?

Because the module is called AutoinstProfile.

so it is common behavior for everything in given namespace, right?

BTW, I have renamed to a hopefully a better name :)

jreidinger · 2017-10-31T08:22:11Z

test/support/autoinst_profile_examples.rb

+    Y2Storage::AutoinstProfile::SectionWithAttributes.new
+  end
+
+  describe ".new_from_hashes" do


so common method for all sections?

in general I see it as problem when you want to create new section.

What I want to test is that the parent is set always when using new_from_hashes. About the method itself, we have a new_from_hashes and new_from_storage to build sections from the profile or from the storage layer.

What is exactly the problem?

my problem is documentation to see what is important and I have to do when I want to create new section.

coveralls · 2017-10-31T09:16:34Z

Coverage decreased (-0.01%) to 95.575% when pulling 8f3b1ae on imo-issues-context into 315a9c9 on master.

coveralls · 2017-10-31T09:18:29Z

Coverage decreased (-0.01%) to 95.575% when pulling 8f3b1ae on imo-issues-context into 315a9c9 on master.

jreidinger

LGTM, even if documentation for sections can be a bit more newcomers friendly.

imobachgs · 2017-10-31T09:29:34Z

Thanks!

imobachgs force-pushed the imo-issues-context branch 2 times, most recently from 407c537 to 6ebe68e Compare October 30, 2017 09:03

yast deleted a comment from coveralls Oct 30, 2017

imobachgs force-pushed the imo-issues-context branch 5 times, most recently from 3a4d1d6 to b790204 Compare October 30, 2017 12:15

imobachgs added 3 commits October 30, 2017 12:56

Add context info to AutoinstProfile classes

8b36d4c

Simplify tests regarding autoinst issues during proposal

5ea16cf

Add context information to AutoinstIssues classes

c1b5e71

imobachgs force-pushed the imo-issues-context branch from b790204 to c1b5e71 Compare October 30, 2017 12:56

Documentation fixes

6ce6595

imobachgs changed the title ~~[WIP] Add context to Autoinst issues~~ Add context to Autoinst issues Oct 30, 2017

yast deleted a comment from coveralls Oct 30, 2017

jreidinger reviewed Oct 31, 2017

View reviewed changes

Updates from code review

8f3b1ae

imobachgs force-pushed the imo-issues-context branch from 07bc51a to 8f3b1ae Compare October 31, 2017 09:12

jreidinger approved these changes Oct 31, 2017

View reviewed changes

imobachgs merged commit 4f63cf7 into master Oct 31, 2017

imobachgs deleted the imo-issues-context branch October 31, 2017 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context to Autoinst issues #400

Add context to Autoinst issues #400

imobachgs commented Oct 27, 2017

coveralls commented Oct 30, 2017

coveralls commented Oct 30, 2017

jreidinger Oct 31, 2017

jreidinger Oct 31, 2017

imobachgs Oct 31, 2017

jreidinger Oct 31, 2017

jreidinger Oct 31, 2017

imobachgs Oct 31, 2017

jreidinger Oct 31, 2017

jreidinger Oct 31, 2017

imobachgs Oct 31, 2017

jreidinger Oct 31, 2017

imobachgs Oct 31, 2017

jreidinger Oct 31, 2017

jreidinger Oct 31, 2017

imobachgs Oct 31, 2017

jreidinger Oct 31, 2017

coveralls commented Oct 31, 2017

coveralls commented Oct 31, 2017

jreidinger left a comment

imobachgs commented Oct 31, 2017

Add context to Autoinst issues #400

Add context to Autoinst issues #400

Conversation

imobachgs commented Oct 27, 2017

coveralls commented Oct 30, 2017

coveralls commented Oct 30, 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

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

coveralls commented Oct 31, 2017

coveralls commented Oct 31, 2017

jreidinger left a comment

Choose a reason for hiding this comment

imobachgs commented Oct 31, 2017