Added LanItems summary #488

teclator · 2017-01-16T09:36:27Z

This is just the firs request for comments, I could make it simpler if needed or if we consider it as over-engineering.

I decided to use the LanItems.summary but permitting to used different types of summaries being the Default based on Summary class as it is in others networks modules like:

https://github.com/yast/yast-network/blob/master/src/modules/Host.rb#L263
https://github.com/yast/yast-network/blob/master/src/modules/DNS.rb#L532
https://github.com/yast/yast-network/blob/master/src/modules/DNS.rb#L532

So for CASP one click dialog we could use:

Yast::LanItems.summary("one_line")
or
Yast::LanItemsSummary.new.one_line

=== OLD ===

Yast::LanItems.summary(:type => "one_line")
or
Yast::LanItemsSummary.new(:type => "one_line").summary
or
Yast::LanItemsSummary::OneLine.new.summary

And in case of a proposal in the future just Yast::LanItems.summary

Here screenshots with the current CASP one dialog overview: yast/yast-installation#487

With one interface configured

With multiple interfaces and different bootproto

coveralls · 2017-01-16T09:39:18Z

Coverage decreased (-0.1%) to 48.542% when pulling 7ce2bac on teclator:lan_items_summary into 027fc19 on yast:master.

imobachgs · 2017-01-16T10:12:37Z

I'm fine with the general approach (having a new class that takes care of generating the summary), but I would simplify it a little bit:

Using classes for each type of summary looks overkill to me (after all, logic is not that complex and you are not maintaining state). Maybe I would implement different methods and that's all.
I would avoid using a generic @options hash. I rather prefer to use real parameters (using keywords, for example). If we're going to have a lot of options, then @options would be a reasonable approach.

Just my opinion :)

mchf · 2017-01-16T10:19:06Z

for the first view it seems to be a bit over engineered, I'll give it even second look ;-)

teclator · 2017-01-16T10:27:02Z

I tried to make it general and as much open as possible. For example with the OneLine type currently we only show on interface or multiple interfaces, but maybe we could change the number of the items to show per line being an option to pass, but don't like to use as a specific option in LanItem, but as commented.

The current types are mostly like presenters, I could by now instead of create separate class just call LanItemsSummary.send(@summary_type) and move the logic to methods as @imobachgs has suggested.

mvidner · 2017-01-16T12:32:47Z

src/lib/network/lan_items_summary.rb

+# You should have received a copy of the GNU General Public License along
+# with this program; if not, contact SUSE LLC.
+#
+# To contact SUSE LLC about this file by physical or electronic mail, you may


The license is trunca

coveralls · 2017-01-16T12:43:32Z

Coverage decreased (-0.1%) to 48.542% when pulling 7a182d9 on teclator:lan_items_summary into 027fc19 on yast:master.

coveralls · 2017-01-16T12:48:15Z

Coverage decreased (-0.1%) to 48.542% when pulling 08e146b on teclator:lan_items_summary into 027fc19 on yast:master.

teclator · 2017-01-16T17:14:57Z

@mchf @mvidner ok, as no updates I will modify current PR based on @imobachgs review.

coveralls · 2017-01-17T10:13:43Z

Coverage decreased (-0.1%) to 48.581% when pulling f51c832 on teclator:lan_items_summary into 027fc19 on yast:master.

imobachgs · 2017-01-17T10:56:54Z

src/lib/network/lan_items_summary.rb

+
+      LanItems.Items.each do |item, conf|
+        next if !Yast::LanItems.IsItemConfigured(item)
+


Why do you separate the body with blank lines?

np: A more elegant solution (in my opinion) would be using reduce (please, feel free to ignore this comment as it's just a matter of taste).

imobachgs · 2017-01-17T10:58:59Z

src/lib/network/lan_items_summary.rb

+      Summary.DevicesList(items)
+    end
+
+    # Generates a one line text summary showing the .


Showing what?

imobachgs · 2017-01-17T11:02:10Z

test/lan_items_summary_test.rb

+      0 => { "ifcfg" => "eth0" },
+      1 => { "ifcfg" => "eth1" },
+      2 => { "ifcfg" => "br0" }
+    }


imobachgs · 2017-01-17T11:02:31Z

test/lan_items_summary_test.rb

+  end
+
+  describe "#default" do
+    it "retuns a Richtext summary of the configured interfaces" do


imobachgs · 2017-01-17T11:03:58Z

test/lan_items_summary_test.rb

+
+    context "when there are no configured interfaces" do
+      let(:items) { {} }
+      it "returns 'Not configured'" do


Above you're referencing to Yast::Summary.NotConfigured. Please, keep consistency.

Yes, but Summary.NotConfigured returns richtext

Yast::Summary.NotConfigured => <ul><li>Not configured yet.</li></ul>

Maybe I'm missing something, but looking at its definition:

Yast.import "Summary" Yast::Summary.NotConfigured #=> "Not configured yet."

Sorry, I was thinking in the previous output that was based on Yast::Summary.DevicesList([]), updated! and thnx for check it.

kobliha · 2017-01-17T12:37:54Z

src/lib/network/lan_items_summary.rb

+      when 1
+        output << configured.join(", ")
+      else
+        output << "Multiple Interfaces"


PLS mark text for translators

imobachgs · 2017-01-17T13:48:06Z

test/lan_items_summary_test.rb

  end
  let(:no_interfaces) { _("Not configured") }
+  let(:multiple_interfaces) { _("Multiple Interfaces") }


np: Maybe these ones are just constants.

coveralls · 2017-01-17T14:42:59Z

Coverage increased (+0.3%) to 48.581% when pulling 9a42b69 on teclator:lan_items_summary into bdd94f3 on yast:master.

Added LanItems summary

7ce2bac

mvidner reviewed Jan 16, 2017

View reviewed changes

teclator force-pushed the lan_items_summary branch from 7a182d9 to 08e146b Compare January 16, 2017 12:45

teclator force-pushed the lan_items_summary branch 2 times, most recently from 6119116 to f582f05 Compare January 16, 2017 13:04

Fix license.

f582f05

Changes based on code review.

f51c832

imobachgs requested changes Jan 17, 2017

View reviewed changes

kobliha reviewed Jan 17, 2017

View reviewed changes

teclator force-pushed the lan_items_summary branch from 1f069a1 to dcf3477 Compare January 17, 2017 13:20

imobachgs reviewed Jan 17, 2017

View reviewed changes

teclator force-pushed the lan_items_summary branch 4 times, most recently from 8c74e79 to 9a42b69 Compare January 17, 2017 14:40

Updated based on CR.

9a42b69

imobachgs approved these changes Jan 17, 2017

View reviewed changes

teclator changed the title ~~[RFC] Added LanItems summary~~ Added LanItems summary Jan 17, 2017

teclator changed the base branch from master to SLE-12-SP2 January 17, 2017 15:17

teclator changed the base branch from SLE-12-SP2 to master January 17, 2017 15:18

teclator mentioned this pull request Jan 17, 2017

Added LanItemsSummary as part of #Fate322328 #489

Merged

teclator closed this Jan 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added LanItems summary #488

Added LanItems summary #488

teclator commented Jan 16, 2017 •

edited

coveralls commented Jan 16, 2017

imobachgs commented Jan 16, 2017

mchf commented Jan 16, 2017

teclator commented Jan 16, 2017

mvidner Jan 16, 2017

teclator Jan 16, 2017

coveralls commented Jan 16, 2017

coveralls commented Jan 16, 2017

teclator commented Jan 16, 2017

coveralls commented Jan 17, 2017

imobachgs Jan 17, 2017

imobachgs Jan 17, 2017

imobachgs Jan 17, 2017

imobachgs Jan 17, 2017

imobachgs Jan 17, 2017

imobachgs Jan 17, 2017

teclator Jan 17, 2017

imobachgs Jan 17, 2017 •

edited

teclator Jan 17, 2017 •

edited

kobliha Jan 17, 2017

teclator Jan 17, 2017

imobachgs Jan 17, 2017 •

edited

coveralls commented Jan 17, 2017


		LanItems.Items.each do \|item, conf\|
		next if !Yast::LanItems.IsItemConfigured(item)

Added LanItems summary #488

Added LanItems summary #488

Conversation

teclator commented Jan 16, 2017 • edited

With one interface configured

With multiple interfaces and different bootproto

coveralls commented Jan 16, 2017

imobachgs commented Jan 16, 2017

mchf commented Jan 16, 2017

teclator commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 16, 2017

coveralls commented Jan 16, 2017

teclator commented Jan 16, 2017

coveralls commented Jan 17, 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

imobachgs Jan 17, 2017 • edited

Choose a reason for hiding this comment

teclator Jan 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imobachgs Jan 17, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Jan 17, 2017

teclator commented Jan 16, 2017 •

edited

imobachgs Jan 17, 2017 •

edited

teclator Jan 17, 2017 •

edited

imobachgs Jan 17, 2017 •

edited