yast / yast-yast2

YaST module yast2
http://en.opensuse.org/Portal:YaST
GNU General Public License v2.0
55 stars 44 forks source link

Add a basic set of classes to register and present errors #1156

Closed imobachgs closed 3 years ago

imobachgs commented 3 years ago

This PR is a follow-up of the discussion in https://github.com/yast/yast-network/pull/1204. It adds a set of simple classes to register and report errors. This API looks pretty much the same, but there are some internal differences.

The only noticeable change when it comes to the API is the concept of "location", which represents where the error was detected (a configuration file, an element of the AutoYaST profile, etc.). At this point, it is just a string, but the plan is to have a better representation so you can easily group errors by location when presenting them to the user. However, the location is not mandatory.

Error Reporting in the Network Module ![error-reporting](https://user-images.githubusercontent.com/15836/114977787-30ffb900-9e80-11eb-9b16-33a686ed8dc9.png)
Error Reporting in the Network Module (multiple errors) ![error-reporting](https://user-images.githubusercontent.com/15836/115024267-c23e5200-9eb7-11eb-9b97-d580dfe43359.png)
Code Example: Adding Issues ```ruby require "yast" require "yast2/issues" issues = Y2Issues::List.new # adding a generic issue issues << Y2Issues::Issue.new("something went wrong", severity: :fatal) # a generic error # adding a more specific issue issues << Y2Issues::InvalidValue( "magic", fallback: "auto", location: "file:/etc/sysconfig/network/ifcfg-eth0:BOOTPROTO" ) puts Yast2::Issues::Presenter.new(errors).to_plain ```

This PR

Near future

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.3%) to 40.821% when pulling ee129f1d569f3401c6931229b960f7fd485a3c10 on yast2-issues into 4f0560a55f0b4ece41ed50063fd7baf00cd8eedc on master.

jreidinger commented 3 years ago

in general I do not like much that so much nesting even for basic use case like in your example. What about Y2Issues namespace instead of Yast2::Issues ? Regarding that widget, maybe I prefer popup class that is premate to be used, but of course it should be able to use directly presenter and prepared dialog only if user explicitly want to show it.

imobachgs commented 3 years ago

in general I do not like much that so much nesting even for basic use case like in your example. What about Y2Issues namespace instead of Yast2::Issues ?

I considered these classes to be "utility classes", and for that reason, I decided to put them there. However, I am fine with moving them to Y2Issues.

Regarding that widget, maybe I prefer popup class that is premate to be used, but of course it should be able to use directly presenter and prepared dialog only if user explicitly want to show it.

Yes, the idea is to offer something ready to be used and, if you want something different, you have the pieces to build it for your use case.

jreidinger commented 3 years ago

In general I like it, just few final notes :)

yast-bot commented 3 years ago

:heavy_check_mark: Public Jenkins job #295 successfully finished :heavy_check_mark: Created OBS submit request #886566