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

logging: rotating handlers set namer and rotator null #82897

Closed
l0rb mannequin opened this issue Nov 6, 2019 · 8 comments
Closed

logging: rotating handlers set namer and rotator null #82897

l0rb mannequin opened this issue Nov 6, 2019 · 8 comments

Comments

@l0rb
Copy link
Mannequin

l0rb mannequin commented Nov 6, 2019

BPO 38716
Nosy @vsajip, @l0rb
PRs
  • bpo-38716: stop rotating handlers from setting inherited namer and rotator to None #17072
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2019-11-07.12:46:50.723>
    created_at = <Date 2019-11-06.09:35:12.436>
    labels = []
    title = 'logging: rotating handlers set namer and rotator null'
    updated_at = <Date 2019-11-07.12:46:50.722>
    user = 'https://github.com/l0rb'

    bugs.python.org fields:

    activity = <Date 2019-11-07.12:46:50.722>
    actor = 'vinay.sajip'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-11-07.12:46:50.723>
    closer = 'vinay.sajip'
    components = []
    creation = <Date 2019-11-06.09:35:12.436>
    creator = 'lorb'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38716
    keywords = ['patch']
    message_count = 8.0
    messages = ['356107', '356112', '356117', '356119', '356121', '356140', '356142', '356156']
    nosy_count = 2.0
    nosy_names = ['vinay.sajip', 'lorb']
    pr_nums = ['17072']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue38716'
    versions = []

    @l0rb
    Copy link
    Mannequin Author

    l0rb mannequin commented Nov 6, 2019

    When subclassing the rotating handlers of the logging module and implementing a namer or rotator method they are silently set to None in __init__. This is surprising behaviour and currently the set a specific namer or rotator it is required to create an instance first and than set them.

    @vsajip
    Copy link
    Member

    vsajip commented Nov 6, 2019

    The namer and rotator attributes are callables, not methods to be overridden. You can certainly do this with methods and set them accordingly:

    class MyHandler(BaseRotatingHandler):
        def __init__(self, *args, **kwargs):
            super(MyHandler, self).__init__(*args, **kwargs)
            self.namer = self.my_namer
            self.rotator = self.my_rotator
    
        def my_namer(self, default_name):
            return default_name  # or whatever you want here
    
        def my_rotator(self, source, dest):
            os.rename(source, dest)  # or whatever you want here

    Having namer and rotator be callables avoids the need to subclass a handler just to override naming and rotating functionality. So, I think this issue should be closed as "not a bug", and the corresponding PR closed. Thanks for your effort, though - I just think you may have misunderstood the intent of the design.

    @l0rb
    Copy link
    Mannequin Author

    l0rb mannequin commented Nov 6, 2019

    Thanks for the review and response. I don't understand yet why not to
    allow the following while keeping the existing functionality. This
    could also be achieved by turning the namer and rotator into class
    attributes. I recently created a subclass of the timed rotating
    handler where it is beneficial for the handler to be able to access to
    self to use the rollover time in the naming.

    class MyHandler(BaseRotatingHandler):
        def namer(self, default_name):
            return default_name  # or whatever you want here
    
        def rotator(self, source, dest):
            os.rename(source, dest)  # or whatever you want here

    Am Mi., 6. Nov. 2019 um 11:35 Uhr schrieb Vinay Sajip <report@bugs.python.org>:

    Vinay Sajip <vinay_sajip@yahoo.co.uk> added the comment:

    The namer and rotator attributes are callables, not methods to be overridden. You can certainly do this with methods and set them accordingly:

    class MyHandler(BaseRotatingHandler):
    def __init__(self, *args, **kwargs):
    super(MyHandler, self).__init__(*args, **kwargs)
    self.namer = self.my_namer
    self.rotator = self.my_rotator

    def my_namer(self, default_name):
        return default_name  # or whatever you want here
    
    def my_rotator(self, source, dest):
        os.rename(source, dest)  # or whatever you want here
    

    Having namer and rotator be callables avoids the need to subclass a handler just to override naming and rotating functionality. So, I think this issue should be closed as "not a bug", and the corresponding PR closed. Thanks for your effort, though - I just think you may have misunderstood the intent of the design.

    ----------
    nosy: +vinay.sajip


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue38716\>


    @vsajip
    Copy link
    Member

    vsajip commented Nov 6, 2019

    They can be methods, but don't need to be. In my example, I showed how you can set the namer and rotator attributes to methods which do have access to self. I don't want to encourage the idea that namer and rotator *have* to be methods that have to be overridden. You can do this in your own code, but I don't especially want to encourage that way of doing things - which is why I specifically didn't make them (namer and rotator) methods.

    @l0rb
    Copy link
    Mannequin Author

    l0rb mannequin commented Nov 6, 2019

    This PR certainly does not turn them into methods. I believe it also
    doesn't encourage the idea that they would have to be. All it does is
    preventing __init__ from setting them to None in the case they are
    defined as methods. Would you consider it more acceptable to just turn
    them into class level attributes of BaseRotatingHandler instead of
    setting them in init?

    @vsajip
    Copy link
    Member

    vsajip commented Nov 6, 2019

    Would you consider it more acceptable to just turn
    them into class level attributes of BaseRotatingHandler instead of
    setting them in init?

    Yes, that would be better.

    @l0rb
    Copy link
    Mannequin Author

    l0rb mannequin commented Nov 6, 2019

    I have changed the PR accordingly.

    Am Mi., 6. Nov. 2019 um 16:36 Uhr schrieb Vinay Sajip <report@bugs.python.org>:

    Vinay Sajip <vinay_sajip@yahoo.co.uk> added the comment:

    > Would you consider it more acceptable to just turn
    them into class level attributes of BaseRotatingHandler instead of
    setting them in init?

    Yes, that would be better.

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue38716\>


    @vsajip
    Copy link
    Member

    vsajip commented Nov 6, 2019

    New changeset 519cb87 by Vinay Sajip (l0rb) in branch 'master':
    bpo-38716: stop rotating handlers from setting inherited namer and rotator to None (GH-17072)
    519cb87

    @vsajip vsajip closed this as completed Nov 7, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    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

    No branches or pull requests

    1 participant