[GRASS-dev] [GRASS-SVN] r64850 - in grass-addons/grass7/gui/wxpython: . wx.mwprecip

GRASS GIS GRASS GIS developer
1

Well, this is interesting, I’m looking forward how this will develop.

Here is code review of some minor issues.

On Fri, Mar 13, 2015 at 1:24 PM, <svn_grass@osgeo.org> wrote:

  • def loadSettings(self, sett=None):
  • if sett:
  • self.settings = sett
  • try:
  • self.database.SetValue(self.settings[‘database’])
  • except:
  • print ‘err’
  • pass
  • try:
  • self.schema.SetValue(self.settings[‘schema’])
  • except:
  • pass
  • try:
  • self.host.SetValue(self.settings[‘host’])
  • except:
  • pass
  • try:
  • self.user.SetValue(self.settings[‘user’])
  • except:
  • pass
  • try:
  • self.port.SetValue(self.settings[‘port’])
  • except:
  • pass
  • try:
  • self.passwd.SetValue(self.settings[‘passwd’])
  • except:
  • pass

This should be one big try-except. Also, except: is a bad practice, this will catch everything including perhaps even SyntaxError. You should specify the error/exception type. You should also specify in the comment why it is OK to ignore the error. What you should use in case of eval that’s a questions, more checks with eval is always a good idea.

Also, please use

if name == ‘main’:

for consistency with other modules. This will allow you to import the files and avoid code duplication between wx..py file and g.gui..py in case the wx.*.py file is needed.

Try to check your files with pep8 before submitting.

2

Vaclav Petras wrote:

This should be one big try-except. Also, `except:` is a bad practice, this
will catch everything including perhaps even SyntaxError. You should
specify the error/exception type.

If you want to catch all errors, use "except StandardError:". That
includes anything which is normally considered an error but avoids
catching SystemExit (generated by sys.exit()) and KeyboardInterrupt
(Ctrl-C etc).

Catching a more specific error is preferable, but not always possible;
the use of duck-typing means that the set of exceptions which a
function can raise depends entirely on the values of the parameters
and variables which it references.

+ try:
+ self.database.SetValue(self.settings['database'])

If the intent is simply to handle the case where the key isn't present
(which would normally raise KeyError), it would probably be better to
just use self.settings.get('database') instead (this returns None if
the key isn't present).

What you should use in case of `eval` that's a questions, more
checks with `eval` is always a good idea.

Avoiding eval() altogether is usually a good idea. To read literal
values of basic types, use ast.literal_eval(). This will evaluate
literal expressions comprised of strings, numbers, booleans, None,
tuples, lists and dictionaries, but won't invoke arbitrary
function/method calls.

--
Glynn Clements <glynn@gclements.plus.com>