Skip to content

Conversation

fprem
Copy link
Collaborator

@fprem fprem commented Mar 15, 2019

No description provided.

Copy link

@lucagiove lucagiove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have to review the core parts

"""

def check_if_exists_in_database(self, selectStatement, sansTran=False):
def check_if_exists_in_database(self,alias, selectStatement, sansTran=False):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enable some PEP8 plugin to remark missing or double spaces.

Suggested change
def check_if_exists_in_database(self,alias, selectStatement, sansTran=False):
def check_if_exists_in_database(self, alias, selectStatement, sansTran=False):

def check_if_exists_in_database(self, selectStatement, sansTran=False):
def check_if_exists_in_database(self,alias, selectStatement, sansTran=False):
"""
Check if any row would be returned by given the input `selectStatement`. If there are no results, then this will

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd better have to document this change also because this is a breaking change that introduces a new mandatory field.

return allRows
return 'DONE',allRows
except Exception as Err:
logger.info('Error: %s' % Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this way you won't stop execution but if an exception occurs during db connection I would not proceed in what am I doing.

Moreover it looks like an error probably should be

Suggested change
logger.info('Error: %s' % Err)
logger.error('Error: %s' % Err)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case of connection problems, the exception is handled in the other class.
Modified the queries in this way, it is possible to manage even the negative tests, not only the positive ones, using robots through the status that is output.

raise AssertionError(err_msg)


def connect_to_database_using_custom_params(self, alias, dbapiModuleName=None, db_connect_string=''):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point of duplicating the method, logging can be added anyway in the called method

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with only one connection there would be no need. With multiple connections I thought it was more correct to add to the error message also the name of the connection that produced it.

@@ -0,0 +1,3 @@
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be in .gitignore

@fprem fprem merged commit bfdc765 into master Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants