https://bugzilla.novell.com/show_bug.cgi?id=798348 https://bugzilla.novell.com/show_bug.cgi?id=798348#c15 --- Comment #15 from Marius Tomaschewski <mt@suse.com> 2013-01-23 13:01:13 UTC --- (In reply to comment #12)
The patch is not bad but I have some comments:
Sure. I'm completely not familiar with all the yast2 mechanisms.
* First of all, we use GitHub now, where cloning and creating pull requests make it easier for us to incorporate changes from different authors easily - this also makes it easy to comment the change
OK, done. I've splitted the yast2 patch into two separate commits (for Service only and for NetworkService changes) and created github pull requests: https://github.com/yast/yast-yast2/pull/44 https://github.com/yast/yast-network/pull/47
* Some pieces changes or even parts of the diff belong to Service YCP module instead of NetworkService - they are quite generic or at least could be For instance Service::RunInitScript() could be used or even extended ("--no-pager -p Id show" as a param)
I didn't wanted to break other services. AFAIS NetworkService reimplements e.g. an own RunInitScript anyway, so there were things to cleanup already before.
* Functions like GetServiceId or GetUnitId definitely belong to Service module
They are in Service.
* Additionally, testsuite is missing
Yes, but I've no idea how to make it the right way. So the testsuites and cleanup or "making it the right way" remains as TODO for the Authors / yast2-maintainers. I'm going to retest & submit the other packages now. -- Configure bugmail: https://bugzilla.novell.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug.