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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary code to figure out logger name #839

Merged
merged 2 commits into from Feb 14, 2018
Merged

Remove unnecessary code to figure out logger name #839

merged 2 commits into from Feb 14, 2018

Conversation

samj1912
Copy link
Collaborator

@samj1912 samj1912 commented Feb 13, 2018 •
edited

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

Solution

Action

@samj1912 samj1912 requested a review from zas February 13, 2018 15:52
zas
zas approved these changes Feb 13, 2018
View reviewed changes
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

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

@mineo: you were right, we don't need to copy stuff around anymore, which is much better.

@zas zas mentioned this pull request Feb 13, 2018
Merged
@samj1912 samj1912 requested a review from mineo February 13, 2018 17:08
mineo
mineo reviewed Feb 13, 2018
View reviewed changes
picard/log.py Outdated
except Exception:
return sys.exc_info()[2].tb_frame.f_back


class OurLogger(logging.getLoggerClass()):
Copy link
Member

Choose a reason for hiding this comment

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

I think you can get rid of this class completely - at least doing that didn't change any of the messages during picard startup with debug mode enabled.

picard/log.py Outdated
if name.startswith(prefix):
name = name[len(prefix) + 1:].replace(os.path.sep, '.').replace('.__init__', '')
record.name, _ = os.path.splitext(name)
name = pathlib.Path(record.pathname)
Copy link
Member

Choose a reason for hiding this comment

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

It took me a while to figure out what this function does - I see that it strips picard from the beginning of a path, removes the file extension and any occurences of .__init__ and for plugins adds the whole path to the file at the beginning of the line, right? In that case, sure, looks reasonable.

Note that before this change, plugin logs were like

/home/wieland/.config/MusicBrainz/Picard/plugins/keep.transltag:19:

now they're like

home.wieland..config.MusicBrainz.Picard.plugins.keep.transltag:19:

Copy link
Collaborator

@zas zas Feb 13, 2018 •
edited

Choose a reason for hiding this comment

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

Note that before this change, plugin logs were like

/home/wieland/.config/MusicBrainz/Picard/plugins/keep.transltag:19:

now they're like

home.wieland..config.MusicBrainz.Picard.plugins.keep.transltag:19:

Not very fancy, that's only for plugins right ?
Can you post a bigger part of your log ?

EDIT: ignore, i didn't see @samj1912 already fixed it

@samj1912 samj1912 force-pushed the simple_logging branch 2 times, most recently from 3b13823 to 02c78eb Compare February 13, 2018 19:21
@samj1912 samj1912 changed the title Remove unnecessary code and use pathlib to figure out logger name Remove unnecessary code to figure out logger name Feb 13, 2018
@zas zas merged commit 601d2ba into metabrainz:master Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mineo mineo mineo left review comments

@zas zas zas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@samj1912 @mineo @zas