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

Refactored logging.ColoredFormatter to avoid deepcopy. #7962

Merged
merged 4 commits into from Aug 23, 2022

Conversation

Julian-O
Copy link
Contributor

@Julian-O Julian-O commented Aug 16, 2022

This is a clean up of kivy.logger code, as described in #7959 It has no effect to the way log files appear or how they are used. It is an internal refactor.

It has an incidental effect of no longer using deepcopy() on logRecords, so it fixes #7528 and #7585.

Maintainer merge checklist

  • Title is descriptive/clear for inclusion in release notes.
  • Applied a Component: xxx label.
  • Applied the api-deprecation or api-break label.
  • Applied the release-highlight label to be highlighted in release notes.
  • Added to the milestone version it was merged into.
  • Unittests are included in PR.
  • Properly documented, including versionadded, versionchanged as needed.

Removed old formatter and support code.
Added 3 LogRecord shims, new formatter, new unit tests for above and a unit test that used to fail to confirm bugs have been fixed.
Match project-style.
(I can't run `black` on these files without
making the review too hard.)
Note to self: Do a `black` refactor of key files so I don't get stuck in this loop again.
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Hi @Julian-O !
LGTM. Thank you!

I've just some minor things that may be addressed before the merge:

  • copy is still imported even if it's not used anymore
  • randint is still imported even if it's not used anymore (not your fault)

Also (and could be fixed later), I've seen that there's still a try/except for Python 2 compat, feel free to remove it, as we don't support Python 2 anymore.

@Julian-O
Copy link
Contributor Author

I agree with everything you said - and I have already fixed them all, and more, in the next PR I have planned.

Can we please merge this one now if I promise to give you a PR with those changes in the next couple of days? I am trying to balance giving you small, incremental improvements that are self-contained and easy-to-review versus how long it will take to get through the backlog (on the order of 7 more PRs?) with review delays between each one. (Happy to hear advice on this.)

@misl6
Copy link
Member

misl6 commented Aug 23, 2022

I am trying to balance giving you small, incremental improvements that are self-contained and easy-to-review
I like it 💯

Yeah, the only one which is really related is copy, but if you promise to remove it in a while (and you promised it 😄 ), there's no issue to get it merged now.

Thank you again!

@misl6 misl6 added the Component: core-app app, clock, config, inspector, logger, resources, modules, clock, base.py label Aug 23, 2022
@misl6 misl6 added this to the 2.2.0 milestone Aug 23, 2022
@misl6 misl6 merged commit c5ff6db into kivy:master Aug 23, 2022
@Julian-O Julian-O deleted the #7959RefactorColoredFormatters branch August 23, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: core-app app, clock, config, inspector, logger, resources, modules, clock, base.py
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logger does deepcopy of record which fails in some environments
2 participants