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

tickets/DM-2299 #5

Merged
merged 2 commits into from Sep 23, 2015
Merged

tickets/DM-2299 #5

merged 2 commits into from Sep 23, 2015

Conversation

andy-slac
Copy link
Contributor

Revisit whether we need something better than a very basic db connection pool.

This module is a wrapper around MySQLdb. It contains a set of low level basic
database utilities such as connecting to database. It caches connections, and
handles database errors.
This module wraps SQLAlchemy.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a confusing statement. We do not wrap whole sqlalchemy here, only one method which instantiates engine.

str(e.orig.args[0]), e.orig.args[1])

#### Loading sql script ############################################################
def loadSqlScript(scriptPath, **kwargs):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should get rid of this method entirely, I don't think we use it anywhere and it's kind of problematic code.

Copy link
Contributor

Choose a reason for hiding this comment

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

We use it in dax_metaserv (admittedly in tests only) and in cat (admittedly that code is very rusty and needs a refresh, most will be replaced by the new metaserv)

@jbecla jbecla force-pushed the tickets/DM-2299 branch 3 times, most recently from e56f38c to ec0839f Compare September 11, 2015 20:03
if fileName.startswith('~'):
fileName = os.path.expanduser(fileName)
if not os.path.isfile(fileName):
raise IOError
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs some arguments like ("File %s does not exist" % fileName), but I would leave that check to ConfigParser.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did leave it to the ConfigParser first, but it throws "NoSectionError: No section: 'database'" which I found too misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, according to the docs parser.read() is used to load optional files, so it does not throw an exception when file is missing, but instead returns the list of files that were loaded. SO I think we should replace parser.read(fileName) with:

parser.readfp(open(fileName), fileName)

and this should take care of the missing file (I think this is better than os.path.isfile() because there may be case when file exists but it's not readable)

This module contains small utilities / helpers that perform common tasks related to
the Db wrapper.

This module contains utilities / helpers that perform common tasks, in particular these that are specific to different drivers.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrap this line

@jbecla jbecla merged commit e306f14 into master Sep 23, 2015
@ktlim ktlim deleted the tickets/DM-2299 branch August 25, 2018 03:43
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.

None yet

4 participants