Skip to content
This repository has been archived by the owner on Oct 25, 2019. It is now read-only.

import person roles #114

Merged
merged 3 commits into from
Feb 14, 2018
Merged

import person roles #114

merged 3 commits into from
Feb 14, 2018

Conversation

de-code
Copy link
Collaborator

@de-code de-code commented Feb 12, 2018

Pre-requisite for #113

This will import the person roles from the eJP XML dumps (it won't use it yet).

(This may actually be deactivated again at some point because we are planning to have a more complte CSV export with that information as well)

@de-code de-code requested a review from seanwiseman February 12, 2018 16:30
@@ -690,7 +695,7 @@ def main():

process_files_in_directory_or_zip(source, process_zip, ext=".zip")

logging.getLogger(NAME).info("done")
get_logger().info("done")

Choose a reason for hiding this comment

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

Would normally just define the logger at the top of the file and use that constant throughout that module scope.

LOGGER = logging.getLogger(__name__)

It is a more common pattern, but preferences aside, also reduces multiple calls to logging.getLogger(__name__).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the module level? Wouldn't that cause issues with the logging configuration? Something like the logger being initialised the first time getLogger is called. I'm sure I observed something which made me change to this pattern (after consulting 'the internet'). Although I couldn't replicate it now so not sure what it was.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I raised a separate issue for that: #117

Choose a reason for hiding this comment

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

When you call logging.getLogger(__name__) in this module, it is returning back a reference to the same logger object every time (the one created for importDataToDatabase in this case) regardless to when you call it.

Choose a reason for hiding this comment

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

Ah ok just seen you comment regarding #117 👍

@de-code de-code merged commit c0a54cc into develop Feb 14, 2018
@de-code de-code deleted the import-person-role branch February 14, 2018 10:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants