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

refactor: use delphi_utils.logger instead of copied file #1488

Merged
merged 3 commits into from
Jul 17, 2024
Merged

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Jul 8, 2024

Addresses the comment here. Partial work from #1469.

Summary:

  • use delphi_utils.logger everywhere instead of the copied logger.py file
  • remove duplicate logger.py in this repo

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

other than the unrelated whitespace changes, this looks fab

src/acquisition/covidcast/database.py Show resolved Hide resolved
Copy link

@dshemetov dshemetov requested a review from melange396 July 17, 2024 17:41
@dshemetov
Copy link
Contributor Author

dshemetov commented Jul 17, 2024

I rebased to get recent client changes and separated the whitespace changes in another commit and ignored that commit in blame.

@melange396 melange396 merged commit b4b9232 into dev Jul 17, 2024
7 checks passed
@melange396 melange396 deleted the ds/logger2 branch July 17, 2024 21:55
@melange396
Copy link
Collaborator

whoops, the whitespace changes are still showing in blame: https://github.com/cmu-delphi/delphi-epidata/blame/dev/src/acquisition/covidcast/database.py#L125

i didnt realize it before i merged, but i think your force push changed the commit hashes :(

dshemetov added a commit that referenced this pull request Jul 17, 2024
Ignore the right blame commit this time. See #1488 (comment).
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