bpo-44121: Implemented formatHeader and formatFooter in BufferingFormatter of the Logging package.

[wiseaidev]

Signed-off-by: Harmouch101 [email protected]

https://bugs.python.org/issue44121

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@Harmouch101

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

[wiseaidev]

I am not sure about the formatting process of records, but I think from the name of the functions, formatting may look like the following:

header
records
footer

right? If so, I will change the test cases to suit this schema.

[vsajip]

Isn't this change backwards-incompatible, in that code (e.g. in subclasses) that assumed the particular behaviour of these method implementations might break? You are welcome to implement these in a subclass - the reason there is no implementation is precisely that every user could want their own version of how the formatting works.

[wiseaidev]

I don't think this change is backward-incompatible. In fact, you still have the option to override the class methods as follows:

class CustomBufferingFormatter(logging.BufferingFormatter):
    def formatHeader(self, records, /, header=""):
      ########### 
      # code block#
      ###########

    def formatFooter(self, records, /, footer=""):
      ########### 
      # code block#
      ###########

Right, isn't it?

[wiseaidev]

However, I've implemented it as I've done because I just wanted to make it easier for the programmer to provide the header and footer instead of overriding the methods like in the current implementation.

[wiseaidev]

Note that you can also call the format method the way it was before these changes(header and footer are just optional):

f = BufferingFormatter()
f.format(records)

[vsajip]

I don't think this change is backward-incompatible.

How so? Haven't you changed what public methods of a public class return? Specifically, formatHeader() and formatFooter() used to return empty strings - now they don't, isn't that correct? If any existing code relied on these empty values (no matter how unlikely that is), it would now break.

The reason why no specific implementation was provided for these methods was that it's something different to each use case, and I wanted to make the user implement whatever it is they want. There's no guarantee that your replacement implementation is of general utility, is there?

[wiseaidev]

That's right. I think I can make it backward compatible. Hold on a sec.

[vsajip]

Your offhand comment in the commit "cause why not!" suggests that you don't consider this compatibility all that important, but it is.

You've also changed the method signature, so code written to call the methods with header="value" or footer="value" will fail when run on older versions of Python. This incompatibility is not an ideal situation. Can you give some reasoning why you can't use subclassing for this use case, as was originally intended? I would prefer not to merge this, because the intention was for people to use subclassing. If that isn't sufficiently clear, I'm willing to review documentation and/or docstring changes that make it clearer.

[wiseaidev]

offhand comment

My bad. Sometimes my brain enters in an inevitable state of weirdness which triggers the neurons to send an electrical signal with an order of magnitude above the potential level that causes my hands to type this kind of sentence.

but it is.

Of course.

You've also changed the method signature

Is it because the / older versions of Python would fail, or is there an edge case that makes it fail? As you may know, it was added to combine positional-only argument (records) and regular with default value argument(header & footer), which the user doesn't have to provide as a parameter in case of formatting without header and footer.

Can you give some reasoning why you can't use subclassing for this use case, as was originally intended?

Cause there is no clear documentation states that we should write another class that inherits from this class and overrides these two methods. Additionally, as I've mentioned earlier, I just wanted to make it easier for the programmer to use them without creating another subclass.

I'm willing to review documentation and/or docstring changes that make it clearer.

If you are going to add some comments and a little piece of documentation that explain how to use these methods, it would be great!

Now, the question is: Is there a way to fix this PR and make it mergeable? It is totally fine if you don't want to. Do you want me to close this PR gracefully? It is really up to you as a maintainer of this Holy Grail.

[vsajip]

I'll close it, but leave the issue open until the documentation is updated. Perhaps I will add a cookbook recipe. Thanks for your input.