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

Use config option for initializing DB class or not #39

Merged
merged 3 commits into from
Apr 12, 2021
Merged

Conversation

ershockley
Copy link
Contributor

This addresses #34. It introduces a new config option in the utilix config that looks like this:

[utilix]
initialize_db_on_import = true

If the field doesn't exist or initialize_db_on_import=false, then utilix will not initialize a DB class instance.

Since I imagine users will most likely want this feature, a warning statement is printed if the DB class is not instantiated:

(XENONnT_development) bash-4.2$ python -c "import utilix"
Warning: DB class NOT initialized on import. You cannot do `from utilix import db`
If you want to initialize automatically on import, add the following to your utilix config:

[utilix]
initialize_db_on_import=true

@ershockley
Copy link
Contributor Author

@jorana what do you think of something like this? Might help with some of the utilix import issues.

Copy link
Contributor

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

Hi Evan, this is certainly a way to do it. However, I personally dislike verbosity on imports if it can be prevented. Would it be worth just excepting the error raised if the API cannot be reached upon init? I mean adding except (FileNotFoundError, SOMEAPI-ERROR) as e:... on line 11 could do the trick right?

@ershockley
Copy link
Contributor Author

I also cringe a little bit at the verbosity here. What if I make it a logging.DEBUG statement instead of a print?

And you think a try-except block is preferable to a config option?

@JoranAngevaare
Copy link
Contributor

Either way is fine by me, the only to things I would consider nice are:

  • Don't print stuff on regular import unless something is not working.
  • By default set initialize_db_on_import = True also if not specified in the config. You can always point people who need this initialize_db_on_import = False if their case arises, it's an expert use case and otherwise everyone starts getting import errors due to the fact that from utilix import DB does not work anymore.

As those are my two considerations, either implementation (of what we mention above) could do but they are not in line with the current code-changes.

@ershockley
Copy link
Contributor Author

Yeah this sounds good. By default it will initialize, but config option will turn it off. I will implement. Thanks!

@ershockley ershockley merged commit a8f935c into master Apr 12, 2021
@ershockley ershockley deleted the import_issue branch April 12, 2021 14:46
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

2 participants