question(log): remove setup() and destroy() from BaseHandler?

[timreichen]

Is your feature request related to a problem? Please describe.

BaseHandler.setup() and BaseHandler.destroy() seem kinda odd as methods for BaseHandler, especially their names.
They do not have a purpose in the BaseHandler itself, except destroy() being called on [Symbol.dispose].

Describe the solution you'd like

I think BaseHandler should remove setup() and destroy(), FileHandler should have methods called open() and close() instead of setup() and destroy() as that describes better what is happening inside the method.
[Symbol.dispose] should be implemented in FileHandler to call close(). RotatingFileHandler would inherit from FileHandler as it does now and could be overridden as needed.

Describe alternatives you've considered

Leave as is.

[iuioiua]

+1

[kt3k]

-1 to this. Doesn't sound like appropriate reasons for doing breaking changes.

BaseHandler.setup() and BaseHandler.destroy() seem kinda odd as methods for BaseHandler, especially their names.

We shouldn't do breaking changes based on just preferences/tastes.

Breaking changes like these breaks the trust to the standard library in my view.

[timreichen]

-1 to this. Doesn't sound like appropriate reasons for doing breaking changes.

BaseHandler.setup() and BaseHandler.destroy() seem kinda odd as methods for BaseHandler, especially their names.

We shouldn't do breaking changes based on just preferences/tastes.

It is not only about taste, web APIs give some guidance for naming methods and properties. We should orient ourselves towards that. For example dispose() would be better than destroy() because Symbol.dispose exists.

But in this particular case BaseHandler.setup() and BaseHandler.destroy() are empty methods and callbacks (similar to customElements connectedCallback and disconnectedCallback()) for subclasses like FileHandler.
These get called in setup(). But FileHandler.open() can be called manually or inside FileHandler.handle(), if the file is not open yet. Similarly, FileHandler.close() can be called manually and is called on the unload event.

[iuioiua]

To be clear, I'm +1 for removing BaseHandler.setup() and BaseHandler. destroy(), as they're empty methods solely there to be overwritten. IMO that doesn't justify their existence.

Renaming them to .open() and .close() makes more sense than their current names. However, I don't find their current names completely misleading, so my opinion isn't too strong.

[kt3k]

as they're empty methods solely there to be overwritten. IMO that doesn't justify their existence.

This seems suggesting removing all of setup and destroy methods in BaseHandler ConsoleHandler FileHandler and RotatingFileHandler (according to #4435). They are not empty methods for FileHandler and RotatingFileHandler.

[iuioiua]

I'm only fully in favour of removing the BaseHandler methods. Please see my latest comment on that PR.

[kt3k]

ConsoleHandler doesn't implement setup and destroy, instead inherits them from BaseHandler. I don't think it makes sense to remove them from BaseHandler. (All handlers are supposed to implement setup and destory ref.

deno_std/log/setup.ts

Line 25 in 660e78e

handler.setup();

[iuioiua]

Then I think we should also remove ConsoleHandler.setup() and ConsoleHandler.destroy(). Reworking setup() for these changes shouldn't be hard. I think these changes lead to a design that makes more logical sense.