Skip to content

Comments

Draft PR#9

Merged
SarahJohnsonONS merged 9 commits intomainfrom
#4-implement-logger
Jan 25, 2024
Merged

Draft PR#9
SarahJohnsonONS merged 9 commits intomainfrom
#4-implement-logger

Conversation

@SarahJohnsonONS
Copy link
Contributor

@SarahJohnsonONS SarahJohnsonONS commented Jan 9, 2024

Some of the required functionality implemented, but I'm a bit stuck on certain parts. See the comments in logger.py (prefaced with #4 for ease of searching).

Copy link
Contributor

@mikeAdamss mikeAdamss left a comment

Choose a reason for hiding this comment

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

this is a very complex thing that's not really doing what I though it was doing (nobodies fault, the api logger was a rush merge and it shows a bit).

I've tried to leave comments, but broadly:

  • make a script that imports DpLogger and does a bunch of logging statements, there's similar in one of my comments.
  • make sure to configure_logger() in the init. I'm not sure that should be a separate function tbh but we'll figure that out at the real PR.
  • at that point when you run your script it'll be punting json logs into your terminal
  • from there it's just twiddling them into the right structures to meet the DP logging standards.

@mikeAdamss
Copy link
Contributor

@SarahJohnsonONS - had a binge on this and pushed code to it.

Logger should be working and pumping out json lines now and I've sketched how we can test it with a few comments.

can you have a look and get back to me if it doesn't make sense? It's much simpler like this.

@SarahJohnsonONS SarahJohnsonONS marked this pull request as ready for review January 22, 2024 14:52
Copy link
Contributor

@NickPapONS NickPapONS left a comment

Choose a reason for hiding this comment

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

Looks good to me, all I found is a typo! I approved but there's still merge conflicts so should be ready after those are addressed.

@SarahJohnsonONS SarahJohnsonONS merged commit 5f79568 into main Jan 25, 2024
@SarahJohnsonONS SarahJohnsonONS deleted the #4-implement-logger branch January 25, 2024 08:12
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.

3 participants