[yast-devel] Re: [yast-commit] <rest-service> master : system - do reboot/shutdown only in 'production' mode
Some remarks inline * Ladislav Slezak <lslezak@novell.com> [Aug 13. 2009 19:04]:
--- a/plugins/system/app/models/system.rb +++ b/plugins/system/app/models/system.rb @@ -46,11 +46,21 @@ class System case action
when :reboot - Rails.logger.debug 'Rebooting the computer...' - return computer.Reboot == 0 + if ENV['RAILS_ENV'] == 'production' + Rails.logger.debug 'Rebooting the computer...' + return computer.Reboot == 0
'Reboot' seems to be a function. However according to Ruby naming conventions 'Reboot' is a class (first letter upper case).
+ else + Rails.logger.debug "Skipping reboot in #{ENV['RAILS_ENV']} mode" + return true + end
The result of the "when :reboot" case is of boolean type. Let the 'reboot' function also return a boolean and drop the comparison with 0
when :shutdown - Rails.logger.debug 'Shutting down the computer...' - return computer.Shutdown == 0 + if ENV['RAILS_ENV'] == 'production' + Rails.logger.debug 'Shutting down the computer...' + return computer.Shutdown == 0 + else + Rails.logger.debug "Skipping shutdown in #{ENV['RAILS_ENV']} mode" + return true + end
The duplication of the production check is not DRY (don't repeat yourself). Also think about moving the check into the 'reboot' resp. 'shutdown' functions, because this is "where the action is". Klaus --- SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg) -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
On Thu, Aug 13, 2009 at 09:31:29PM +0200, Klaus Kaempf wrote:
* Ladislav Slezak <lslezak@novell.com> [Aug 13. 2009 19:04]:
+ if ENV['RAILS_ENV'] == 'production' + Rails.logger.debug 'Rebooting the computer...' + return computer.Reboot == 0
'Reboot' seems to be a function. However according to Ruby naming conventions 'Reboot' is a class (first letter upper case).
+ else + Rails.logger.debug "Skipping reboot in #{ENV['RAILS_ENV']} mode" + return true + end
The result of the "when :reboot" case is of boolean type. Let the 'reboot' function also return a boolean and drop the comparison with 0
These are valid points, but "computer" is a HAL object (via DBus): computer = hal_service.object('/org/freedesktop/Hal/devices/computer') http://git.opensuse.org/?p=projects/yast/rest-service.git;a=blob;f=plugins/s... - The methods return "Int32 return_code". Not worth wrapping to a boolean, I think. - Converting method names from ruby_style to DbusStyle could work. Should I look into it? -- Martin Vidner, YaST developer http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
* Martin Vidner <mvidner@suse.cz> [Aug 14. 2009 09:07]:
On Thu, Aug 13, 2009 at 09:31:29PM +0200, Klaus Kaempf wrote:
* Ladislav Slezak <lslezak@novell.com> [Aug 13. 2009 19:04]:
+ if ENV['RAILS_ENV'] == 'production' + Rails.logger.debug 'Rebooting the computer...' + return computer.Reboot == 0
'Reboot' seems to be a function. However according to Ruby naming conventions 'Reboot' is a class (first letter upper case).
+ else + Rails.logger.debug "Skipping reboot in #{ENV['RAILS_ENV']} mode" + return true + end
The result of the "when :reboot" case is of boolean type. Let the 'reboot' function also return a boolean and drop the comparison with 0
These are valid points, but "computer" is a HAL object (via DBus): computer = hal_service.object('/org/freedesktop/Hal/devices/computer')
Ah, now this explains it. ;-)
- The methods return "Int32 return_code". Not worth wrapping to a boolean, I think.
Agreed.
- Converting method names from ruby_style to DbusStyle could work. Should I look into it?
Probably not worth it. We just have to take care that such D-Bus objects are properly encapsulated. The current system/app/models/system.rb is a proper encapsulation imho. Klaus --- SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg) -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
Hi, On 13.8.2009 21:31, Klaus Kaempf wrote:
'Reboot' seems to be a function. However according to Ruby naming conventions 'Reboot' is a class (first letter upper case).
This is a HAL DBus method call, it has int org.freedesktop.Hal.Device.SystemPowerManagement.Reboot() function, Ruby conventions cannot be applied here.
+ else + Rails.logger.debug "Skipping reboot in #{ENV['RAILS_ENV']} mode" + return true + end
The result of the "when :reboot" case is of boolean type. Let the 'reboot' function also return a boolean and drop the comparison with 0
Again, it's a HAL method returning int (see above).
The duplication of the production check is not DRY (don't repeat yourself). Also think about moving the check into the 'reboot' resp. 'shutdown' functions, because this is "where the action is".
The same here... -- Best Regards Ladislav Slezák Yast Developer ------------------------------------------------------------------------ SUSE LINUX, s.r.o. e-mail: lslezak@suse.cz Lihovarská 1060/12 tel: +420 284 028 960 190 00 Prague 9 fax: +420 284 028 951 Czech Republic http://www.suse.cz/ -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org
participants (3)
-
Klaus Kaempf -
Ladislav Slezak -
Martin Vidner