Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize usage of log.exception #3933

Merged
merged 4 commits into from Apr 21, 2017

Conversation

Projects
None yet
5 participants
@dannon
Copy link
Member

commented Apr 13, 2017

Standardizes our usage of log.exception to 1) not include the exception in the string, and 2) use log.exception parameters for string templating.

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

Thanks! By the way, the str() castings should be unnecessary.

@dannon

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

Yep, also going to convert usage of log.exception("oops %s" % var) to log.exception("oops %s", var)

(going to make multiple passes to make sure I get each class of error)

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

Fantastic! I was going to suggest it, but I didn't want to be too nitpicky!

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

Also take a look at the flake8 errors, many exception variables are now unused.

dannon added some commits Apr 13, 2017

@martenson martenson changed the title WIP Standardize usage of log.exception [WIP] Standardize usage of log.exception Apr 19, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

@dannon this has both WIP and review labels

@dannon dannon removed the status/WIP label Apr 19, 2017

@dannon dannon changed the title [WIP] Standardize usage of log.exception Standardize usage of log.exception Apr 19, 2017

@galaxybot galaxybot added this to the 17.05 milestone Apr 19, 2017

@jmchilton jmchilton merged commit 386afbb into galaxyproject:dev Apr 21, 2017

5 checks passed

api test Build finished. 274 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 140 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@nsoranzo
Copy link
Member

left a comment

Sorry for late review, I didn't get notification for the label change.

@@ -607,7 +607,7 @@ def traverse( folder ):
log.exception( "Unable to create archive for download" )
raise exceptions.InternalServerError( "Unable to create archive for download." )
except Exception:
log.exception( "Unexpected error %s in create archive for download" % sys.exc_info()[ 0 ] )
log.exception( "Unexpected error %s in create archive for download", sys.exc_info()[ 0 ] )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 21, 2017

Member

This one could probably be refactored even more to just:

log.exception("Unexpected error in create archive for download")
@@ -1863,7 +1863,7 @@ def __str__( self ):
status = 'error'
except:
error = True
log.exception( "Unexpected error %s in create archive for download" % sys.exc_info()[0] )
log.exception( "Unexpected error %s in create archive for download", sys.exc_info()[0] )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 21, 2017

Member

Also this one could probably be refactored even more to just:

log.exception("Unexpected error in create archive for download")
except Exception as e:
log.exception( "Exception in ToolPanelManager.config_elems_to_xml_file: %s" % str( e ) )
except Exception:
log.exception( "Exception in ToolPanelManager.config_elems_to_xml_file: %s" )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 21, 2017

Member

Need to remove : %s here.

# The viewable repository numbers and the categorized (filtered) lists of repository tuples
# may be slightly skewed, but that is no reason to result in a potential server error. All
# will be corrected at next server start.
log.exception( "Handled error removing entry from repository registry: %s." % str( e ) )
log.exception( "Handled error removing entry from repository registry: %s." )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Apr 21, 2017

Member

Need to remove : %s. here.

@martenson

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

@nsoranzo label changes do not notify afaik :'(

@dannon

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

@nsoranzo Thanks for finding these issues, opening PR fixing it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.