Skip to content

Conversation

@cehbrecht
Copy link
Collaborator

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes issue #xyz
  • Tests for the changes have been added (for bug fixes / features)
  • Documentation has been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated (with summary of main changes)
  • I have added my relevant user information to AUTHORS.md
  • What kind of change does this PR introduce?:

This PR updates the logging to use loguru.
This change was introduced in clisops>=0.9.0, see roocs/clisops#216

  • Does this PR introduce a breaking change?:

  • Other information:

@cehbrecht cehbrecht requested a review from Zeitsperre April 12, 2022 13:02
@cehbrecht
Copy link
Collaborator Author

cehbrecht commented Apr 12, 2022

@Zeitsperre this is mainly a copy+paste from roocs/clisops#216. We can probably reduce some duplicates, like the tests._common.ContextLogger.

@Zeitsperre
Copy link
Contributor

@cehbrecht Thanks and looks good! I'm hoping that this new engine solves a few of our problems.

If pytest supported class fixtures, the ConextLogger would be much better off as one. I've written a similar one into xclim (https://github.com/Ouranosinc/xclim/blob/9acbc72e8a22d3267929db3b88080c12b9f01288/xclim/testing/_utils.py#L35) that handles for the behaviour of loguru being enabled when xclim's version ends in beta.

If it can eventually be defined in a lower-level library (like, daops I suppose), we could then remove redundant definitions elsewhere.

@Zeitsperre
Copy link
Contributor

@cehbrecht #93 should be compatible after the merge of this PR.

@cehbrecht cehbrecht merged commit 58805b1 into master Apr 12, 2022
@cehbrecht cehbrecht deleted the update-logging branch April 12, 2022 14:58
@cehbrecht cehbrecht mentioned this pull request Apr 12, 2022
5 tasks
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